[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: Current git HEAD busybox segfaults on some applets
From:       Rich Felker <dalias () libc ! org>
Date:       2016-09-16 16:41:24
Message-ID: 20160916164124.GN15995 () brightrain ! aerifal ! cx
[Download RAW message or body]

On Fri, Sep 16, 2016 at 04:35:23PM +0000, Laurent Bercot wrote:
> 
>  A full gdb output is available here: http://pastebin.com/3k6SENiX
> 
>  The issue comes from the fact that fflush(stream) is #define'd as
> fflush_unlocked(stream), so fflush(0) actually runs
> fflush_unlocked(0), which segfaults with the current version of
> musl (but not glibc).
> 
>  fflush_unlocked() and friends are not part of the standard API
> (only getc_unlocked and putc_unlocked are) and it is a mistake
> to assume they 1. exist, 2. behave the same as their non-unlocked
> counterpart.
> 
>  The culprit is this commit:
> https://git.busybox.net/busybox/commit/include/libbb.h?id=aa3576a29b9619f4e1c1b131f5db53ad2bc2cb00
> 
>  Later commits modify the additions in libbb.h, but those
> additions are incorrect in the first place. I will send a
> patch that removes them.

More to the point, they're unnecessary and unsafe. The *_unlocked
functions are not well-specified, and except for getc/putc, they're
not going to yield you any performance benefits anyway.

The issue here happened because musl's fflush_unlocked is just
provided as an alias for the internal function fflush() calls once the
file is locked, or for each function in the list of all open files
when fflush(0) is used. This internal function is not capable of
handling the fflush(0) behavior itself. Maybe it should be able to;
I'm leaning towards getting rid of all the *_unlocked functions except
the standard ones and making the rest aliases for the non-_unlocked
version just to keep this kind of thing from happening in the future.
But I don't think it's a good idea for applications to be using them.
They were added purely for compatibility with broken software that
assumes they exist, not because they're useful for anything.

Rich
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]