Undefined behavior in glibc -- was: Re: [PATCH][BZ 17979][BZ 17721] Fix issues with sys/cdefs.h and uchar.h when using non-gcc compiler.
Alexander Cherepanov
ch3root@openwall.com
Wed Feb 3 12:29:00 GMT 2016
Joseph,
thanks a lot for the detailed reply. My comments are below.
On 2016-01-31 19:12, Joseph Myers wrote:
> On Sat, 30 Jan 2016, Alexander Cherepanov wrote:
>
>> But we can consider something much more real: glibc code contains many cases
>> of undefined behavior (according to C11). I consider it bugs unwanted in
>> production (maybe with the exception of strlen) but it's not clear to me what
>> is the POV of the project on this. E.g., are bug reports about UB are welcome?
>
> It's an intrinsic part of implementing the C library that it involves
> doing things that are not defined in ISO C (the ISO C library cannot all
> be implemented in ISO C).
But those parts which cannot be implemented in ISO C would be
implemented in asm? AIUI there is no much magic in compiling C parts of
glibc so they should obey the common rules?
> If something involves undefined behavior in GNU C,
GNU C is ISO C (whatever it means:-) + GCC implementation-defined
behavior as described in [1] + GCC extensions[2], right?
[1] https://gcc.gnu.org/onlinedocs/gcc/C-Implementation.html
[2] https://gcc.gnu.org/onlinedocs/gcc/C-Extensions.html
> on systems with the
> properties that hold for all systems supported by glibc (e.g. 32-bit int,
> 32-bit or 64-bit long and pointers), for inputs to library functions where
> the library function semantics don't involve undefined behavior, taking
> due account of both separate compilation and use of asm to reduce the
> information available to the compiler in certain places and so make things
> not undefined that would otherwise be undefined, then I'd consider bug
> reports appropriate. Examples of this include buffer overruns and signed
> integer arithmetic overflow. I think that applies even if there is no
> plausible optimization that could result in the signed integer arithmetic
> overflow causing problems. Of course, patches are even better.
This looks reasonable but I guess there supposed to be some exceptions?
E.g.:
1) strlen and other string functions read memory by chunks which is
aliasing violation;
2) strlen accesses the OOB memory.
AIUI for 1) there is may_alias gcc attribute but it almost never used in
glibc. There are no tools in GNU C to make 2) well-defined but the
speed-up is remarkable (Rich Felker mentioned 3x difference in speed in
musl strlen on his hw).
Documenting all the exceptions would be nice. Then, ASan is in glibc
todo so a clean version of strlen has to be added.
> If something is fully defined in GNU C, but undefined, unspecified or
> implementation-defined in ISO C, I don't think bug reports are
> appropriate. E.g., it's not a bug for glibc code to rely on signed shifts
> by a nonnegative amount that's less than the width of the type, including
> the aspects of those that are not defined by ISO C, or on the
> fully-defined nature of conversions of out-of-range integer values to
> signed integer types. Some such cases in code shared with gnulib (or
> other external sources) may still be gnulib bugs where they aren't glibc
> bugs, since gnulib is meant to be much more portable. And there may be
> cases where you could argue that eliminating such usages is a desirable
> *cleanup* in glibc - e.g., that the code is cleaner when it uses unsigned
> types for bitwise operations. But in such cases there is not bug and so
> patches should be sent without any bug reports in Bugzilla (or more
> general groups of issues identified in the list of possible cleanups in
> the todo list on the wiki).
Ok
Making glibc compilable by clang would be nice too (and it's in the
glibc todo list).
> Where something depends on a property common to all systems supported by
> glibc, again it may be a useful *cleanup* to make this more explicit in
> the code, without there being any *bug* that's appropriate to report to
> Bugzilla. E.g., if code is using int in a way that requires it to be
> exactly 32-bit, changing the code to use int32_t is a sensible cleanup; if
> it really does mean int but is hardcoding the value of INT_MAX, making it
> use INT_MAX is a sensible cleanup; if it's using long as a pointer-sized
> integer type, changing it to use intptr_t is a sensible cleanup.
Ok
> Where glibc code relies on separate compilation to avoid undefined
> behavior, this is not a bug; use of LTO on glibc's own code is not
> supported. For example, where functions with different C types but
> compatible ABIs are aliased, or a function is otherwise defined with a
> different type from that used when it is called. Similarly, asm may be
> used to limit code movement, and that could e.g. mean that aliasing is not
> undefined behavior where it would otherwise be undefined.
Not sure what you mean. Is it specific to calling functions or it's
supposed to cover the case of strlen too?
I don't see cleaning glibc for LTO in todo list in the wiki. Shouldn't
it be there?
> Of course, if the semantics for a function say that certain inputs result
> in undefined behavior, it does not matter if C-level undefined behavior
> occurs within that function for those inputs.
Sure.
Well, before I go and file the bugs, perhaps it's worth skimming through
the list. It's mainly invalid pointer arithmetic. GNU C seems not to
deviate from ISO C in this topic. Should the issues be grouped by type,
by file, by subdir or in some other way?
Pointer wrapping
----------------
Likely to be miscompiled in the future. Already reported:
https://sourceware.org/bugzilla/show_bug.cgi?id=19411
https://sourceware.org/bugzilla/show_bug.cgi?id=19412
https://sourceware.org/bugzilla/show_bug.cgi?id=19413
Pointers pointing past an array
-------------------------------
I guess there are a lot of these but searching for them is not easily
automated so the list below is probably just the tip of the iceberg and
maybe not even accurate.
Sometimes it could be proved that the amount of "overflow" is small and,
thus, it should be safe in practice (given that an object usually could
not be allocated near the end of the address space).
https://sourceware.org/git/?p=glibc.git;a=blob;f=resolv/ns_name.c;h=f355cf34443c46d091157ed06f4ef8487214bf10;hb=HEAD#l548
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_getai.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l113
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_getgr_r.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l240
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_gethst_r.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l215
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_gethst_r.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l370
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_getpw_r.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l168
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_getserv_r.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l139
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_getserv_r.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l292
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_initgroups.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l73
Pointers pointing before an array
---------------------------------
Should be safe in practice?
https://sourceware.org/git/?p=glibc.git;a=blob;f=wcsmbs/wcsncat.c;h=9b22764ca13b16a96b6f32636072203658950fdd;hb=HEAD#l39
https://sourceware.org/git/?p=glibc.git;a=blob;f=wcsmbs/wcsncpy.c;h=cd90024e4410cf06cc1083ec10d6f677f18f3e92;hb=HEAD#l32
https://sourceware.org/git/?p=glibc.git;a=blob;f=string/memcmp.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l131
https://sourceware.org/git/?p=glibc.git;a=blob;f=string/wordcopy.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l42
etc.
Capping pointers at the end of the address space
------------------------------------------------
Some functions in C11 have a parameter which limits a number of
characters written. IMHO it's clear that C11 doesn't intend this
parameter to be a real size of output buffer and, thus, it could validly
be SIZE_MAX. But we have seen differing opinions in the recent thread
about strlcpy. Perhaps you can clear the question.
Anyway, glibc inner parts use a pointer to the end of the buffer and
upper-level functions have to check for pointer wrapping and to cap this
pointer at the end of the address space.
1. Functions like snprintf cap the pointer in _IO_str_init_static_internal.
2. Functions like wcsrtombs have to cap __outbufend in struct
__gconv_step_data.
The capping itself is not UB (if done right) but later pointer
comparisons with the resulting pointer are UB. Probably safe in practice
as it's similar to [7/15] in
https://gcc.gnu.org/ml/gcc/2015-04/msg00325.html .
API change required? :-)
Then, there are less important cases:
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_gethst_r.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l169
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_getai.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l68
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_getgr_r.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l105
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_getpw_r.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l100
https://sourceware.org/git/?p=glibc.git;a=blob;f=nscd/nscd_getserv_r.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l113
and probably others.
Subtracting NULL from a pointer
-------------------------------
Plausible optimization: this is unconditional UB, hence mark the code as
unreachable and remove everything around.
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/sigstack.c;h=b134d8e5b9655aec4afea961e1eb63f558be62c1;hb=HEAD#l45
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/profil.c;h=23e601c6c2d5a677a778df65c9088f34ff1b7a04;hb=HEAD#l40
35 other cases are in the attached subtract-null.patch (generated).
Structs at NULL
---------------
Plausible optimization: this is unconditional UB, hence mark the code as
unreachable and remove everything around.
https://sourceware.org/git/?p=glibc.git;a=blob;f=argp/argp-parse.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l81
https://sourceware.org/git/?p=glibc.git;a=blob;f=include/list.h;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l94
https://sourceware.org/git/?p=glibc.git;a=blob;f=intl/bindtextdom.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l44
https://sourceware.org/git/?p=glibc.git;a=blob;f=intl/dcigettext.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l120
https://sourceware.org/git/?p=glibc.git;a=blob;f=intl/dcigettext.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l125
https://sourceware.org/git/?p=glibc.git;a=blob;f=nptl_db/db_info.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l76
https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/glob.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l151
https://sourceware.org/git/?p=glibc.git;a=blob;f=resolv/nss_dns/dns-network.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l241
https://sourceware.org/git/?p=glibc.git;a=blob;f=stdio-common/reg-modifier.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l58
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/ia64/sys/ucontext.h;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l39
Pointers used after free (without dereferencing)
------------------------------------------------
https://sourceware.org/git/?p=glibc.git;a=blob;f=libio/fileops.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l1063
(IIUC conditions "fp->_IO_read_base != NULL" and "!_IO_in_backup (fp)"
should be swapped.)
https://sourceware.org/git/?p=glibc.git;a=blob;f=malloc/tst-obstack.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l25
https://sourceware.org/git/?p=glibc.git;a=blob;f=malloc/tst-malloc-backtrace.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l30
https://sourceware.org/git/?p=glibc.git;a=blob;f=io/pwd.c;hb=5163b4b76f61e361f0f4bbe3b96732b12e5c9b1a#l41
Memory allocations without check
--------------------------------
Not UB but while we are at it...
AIUI the policy in glibc is to check results of malloc no matter how
small the requested amount of memory is. Right?
So mallocs like in https://sourceware.org/bugzilla/show_bug.cgi?id=19416
should be reported?
--
Alexander Cherepanov
-------------- next part --------------
A non-text attachment was scrubbed...
Name: subtract-null.patch
Type: text/x-patch
Size: 10849 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20160203/85e5f592/attachment.bin>
More information about the Libc-alpha
mailing list