Date: Tue, 27 Sep 2011 17:00:04 -0400 From: Rich Felker <dalias@...ifal.cx> To: musl@...ts.openwall.com Subject: Re: musl bugs On Tue, Sep 27, 2011 at 08:06:46PM +0400, Vasiliy Kulikov wrote: > Hi Rich, > > getmntent_r(): > - fgets() should be checked for too small buffer. And what should happen? ERANGE? This non-standardized stuff is so poorly documented... Should it seek back and allow you to read the entry next time with a larger buffer? Or should it just fail? > - Looks like fgets() may fail. Then ferror() should be used together > with feof(). Yes, I used to mistakenly think errors also set the EOF indicator... > getmntent(): > - Is linebuf big enough? IMO as the buffer is not supplied by a > user, it should be dynamically allocated. Calling getmntent() and > getting truncated result/ERANGE is somewhat not expected. You're probably right... > addmntent(): > - Here fseek() can be easily checked for errors => return 1 in case of > error. Fixing it. > hasmntopt(): > - Implementation is wrong. The argument is not a substring, but a > single option, possibly with "=value". Glibc's implementation is OK > IMO. I will look into this... > prctl() and other places: > - Why no va_end()? It is __builtin_va_end() sometimes, and AFAIU it is > not a noop. Do you have a list? AFAIK it is (and must be) a no-op in the real world, but for correctness we should use it anyway. (An implementation theoretically could malloc as part of va_start/va_arg, but this would render it a junk/useless implementation because variadic functions would crash on OOM..) > getgrgid() and getgrnam(): > - errno is not saved while calling endgrent() (close() inside). POSIX > says close() may return EIO if I/O error happened during close() with > RO fd, altering errno. Also getpwuid and getpwnam... Fixed. > execvp(): > - As the code chooses the first possible path in $PATH, the > /usr/local/bin should be the last path. POSIX says it should start > with null path (current dir), but it is crazy. Where does it say this? I see (in the execvp documentation in POSIX 2008): "If this environment variable is not present, the results of the search are implementation-defined." In particular, unless you use sysconf to obtain and set a default PATH, there's no implication that the standard utilities should be in the search. I put /usr/local/bin first because the idea is that you use it locally to override possibly-shared/distro-provided binaries in /usr/bin and /bin. I'm open to suggestions on changing this if it's problematic, but I don't think it's a conformance issue. > - I don't see an overflow here (comment claims so)... Well there's definitely a VLA overflow, and there could be an arithmetic wrap-around if file points to a substring of getenv("PATH") (pathological but possible). Both issues should be fixed, probably by simply rejecting file longer than NAME_MAX and path components longer than PATH_MAX, and simply using a fixed-size buffer of size PATH_MAX. Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.