musl bugs

musl bugs

[Vasiliy Kulikov]

Hi Rich,

getmntent_r():
- fgets() should be checked for too small buffer.
- Looks like fgets() may fail.  Then ferror() should be used together
  with feof().

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.

addmntent():
- Here fseek() can be easily checked for errors => return 1 in case of
  error.

hasmntopt():
- Implementation is wrong.  The argument is not a substring, but a
  single option, possibly with "=value".  Glibc's implementation is OK
  IMO.

prctl() and other places:
- Why no va_end()?  It is __builtin_va_end() sometimes, and AFAIU it is
  not a noop.

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.

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.
- I don't see an overflow here (comment claims so)...

-- 
Vasiliy

Re: musl bugs

[Rich Felker]

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

Re: musl bugs

[Vasiliy Kulikov]

On Tue, Sep 27, 2011 at 17:00 -0400, Rich Felker wrote:
> 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?

The right thing would be seeking back, but at least glibc simply ignores
the error and seeks to the end of line.  I don't known whether seek back
is better given this behaviour is not documented...  To be consistent
with glibc IMHO it should seek till the EOL, but return the error.


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

Oops, sorry, I've confused "man 3p" and "man 3".  Indeed, POSIX doesn't
define any default path.


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

Hmm, looks like this is a distro specific thing.  Given it is not
standardized, it shouldn't be an issue (unless some broken app relies on
specific path sets).

Thanks,

-- 
Vasiliy

Re: musl bugs

[Rich Felker]

On Wed, Sep 28, 2011 at 11:54:46AM +0400, Vasiliy Kulikov wrote:
> On Tue, Sep 27, 2011 at 17:00 -0400, Rich Felker wrote:
> > 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?
> 
> The right thing would be seeking back, but at least glibc simply ignores
> the error and seeks to the end of line.  I don't known whether seek back
> is better given this behaviour is not documented...  To be consistent
> with glibc IMHO it should seek till the EOL, but return the error.

If implementations differ, a correct program will always need to save
the location and seek back to work reliably. On the other hand, if the
implementation seeks back, it will break applications that want to
just ignore overly-long lines and keep reading (they'll hit an
infinite loop of failures). Also, seeking back the way I do it now
causes breakage even if the long line is just a comment.

Thus I'm leaning towards going with the glibc behavior.

Rich