[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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[256] 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.
