Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 27 Sep 2011 17:00:04 -0400
From: Rich Felker <>
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...

> 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.


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.