1. Forum
  2. Usenet
  3. LINUX.DEBIAN.MAINT.GLIBC

  • Bug#867283: Crash in glibc's mktime in low-memory situations

    From Johannes Schultz@21:1/5 to All on Wed Jul 5 20:50:02 2017
    XPost: linux.debian.bugs.dist

    None of the internal assertions in tzfile.c have to do with low
    memory, they have to do with logical consistency and expected
    outcomes.

    Okay, so let's look at the stack trace again and where it failed.
    The failing line 779 in __tzfile_compute is:

    if (__tzname[0] == NULL)
    {
    assert (num_types == 1); // <-- boom

    So, where is __tzname[0] being set? Depending on the path taken, it can
    be either of these:

    line 627: __tzname[0] = NULL; // initial value
    line 646: __tzname[0] = __tzstring (&zone_names[types[i].idx]);
    line 686: __tzname[0] = __tzstring (zone_names);
    line 756: __tzname[dst] = __tzstring (&zone_names[idx]);

    Internally, __tzstring calls malloc. If malloc fails, it returns NULL.
    So it is entirely possible that this assertion will fail because of an out-of-memory situation.

    No of course this is bad, but so far the integrity has not been
    compromised. It just means that the function really should return with
    an error now. As far as I can see there are currently no facilities for returning an error in this particular function, but I guess it really
    should be able to propagate allocation failures to the library functions
    that call it, so that those can return an error to the user (if that is
    part of their API contract, that is).

    Cheers,
    Johannes

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Johannes Schultz@21:1/5 to All on Wed Jul 5 16:20:02 2017
    XPost: linux.debian.bugs.dist

    Package: glibc
    Version: 2.19-18+deb8u10

    By fuzzing my own software using American Fuzzy Lop and its provided libdislocator (an "abusive allocator" library), I found a code path in
    glibc that causes a SIGABRT where it probably shouldn't.

    In a low-memory situation, I got the following stack trace:
    #0 0x00007ffff6319067 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
    #1 0x00007ffff631a448 in __GI_abort () at abort.c:89
    #2 0x00007ffff6312266 in __assert_fail_base (fmt=0x7ffff644af18
    "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7ffff644893f "num_types == 1", file=file@entry=0x7ffff6448936 "tzfile.c", line=line@entry=779, function=function@entry=0x7ffff644fc90 <__PRETTY_FUNCTION__.6261> "__tzfile_compute") at assert.c:92
    #3 0x00007ffff6312312 in __GI___assert_fail (assertion=assertion@entry=0x7ffff644893f "num_types == 1", file=file@entry=0x7ffff6448936 "tzfile.c", line=line@entry=779, function=function@entry=0x7ffff644fc90 <__PRETTY_FUNCTION__.6261> "__tzfile_compute") at assert.c:101
    #4 0x00007ffff6391907 in __tzfile_compute (timer=1447111074, use_localtime=use_localtime@entry=1, leap_correct=leap_correct@entry=0x7fffffffd848, leap_hit=leap_hit@entry=0x7fffffffd844, tp=tp@entry=0x7fffffffd960) at tzfile.c:779
    #5 0x00007ffff6390429 in __tz_convert (timer=0x7fffffffd948,
    use_localtime=1, tp=0x7fffffffd960) at tzset.c:635
    #6 0x00007ffff638eab0 in ranged_convert (convert=0x7ffff638e940 <__localtime_r>, t=0x7fffffffd948, tp=0x7fffffffd960) at mktime.c:310
    #7 0x00007ffff638edd5 in __mktime_internal (tp=0x7fffffffdab0, convert=0x7ffff638e940 <__localtime_r>, offset=0x7ffff668bab8 <localtime_offset>) at mktime.c:478
    #8 0x00007ffff6c02083 in OpenMPT::mpt::Date::Unix::FromUTC
    (timeUtc=...) at common/mptTime.cpp:115

    mktime is supposed to return -1 and, according to cplusplus.com, has a
    no-throw guarantee for C++ code. So even if some internal memory cannot
    be allocated, I expect mktime to return with an error value and not
    cause a SIGABRT.
    I found it difficult to reproduce this outside my afl-instrumented
    environment, but I hope the stack trace helps with verifying whether
    this is a valid bug. If you need a test case to reproduce, let me know.

    Cheers,
    Johannes

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Carlos O'Donell@21:1/5 to Johannes Schultz on Wed Jul 5 17:00:02 2017
    XPost: linux.debian.bugs.dist

    On Wed, Jul 5, 2017 at 9:13 AM, Johannes Schultz <[email protected]> wrote:
    mktime is supposed to return -1 and, according to cplusplus.com, has a no-throw guarantee for C++ code. So even if some internal memory cannot be allocated, I expect mktime to return with an error value and not cause a SIGABRT.
    I found it difficult to reproduce this outside my afl-instrumented environment, but I hope the stack trace helps with verifying whether this is a valid bug. If you need a test case to reproduce, let me know.

    If an internal assertion in tzfile.c triggers then it means that the
    integrity of the library is compromised.

    None of the internal assertions in tzfile.c have to do with low
    memory, they have to do with logical consistency and expected
    outcomes.

    I've reviewed tzfile.c, tzset.c, and mktime.c and it's hard for me to
    see what low-memory path could trigger the assertion. One possible
    hypothesis is that your program is corrupting library data given
    certain AFL input.

    You will have to review your AFL input in more detail to see why it is
    causing a SIGABRT.

    In general the library should not SIGABRT for normal conditions like low-memory. I agree that if you find that is *really* the case, then
    we can fix this upstream.

    Cheers,
    Carlos.

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Johannes Schultz@21:1/5 to All on Wed Jul 5 23:10:02 2017
    XPost: linux.debian.bugs.dist

    Am 05.07.2017 um 21:56 schrieb Carlos O'Donell:
    I agree.

    If you have the opportunity please file a match bug with upstream at sourceware.org/bugzilla. That way upstream is made aware of the issue.

    Sure, I'll report the bug there.

    Cheers,
    Johannes

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Johannes Schultz@21:1/5 to All on Wed Jul 5 23:30:02 2017
    XPost: linux.debian.bugs.dist

    (Mail was initially only sent to Carlos, sorry for that :))

    If you have the opportunity please file a match bug with upstream at sourceware.org/bugzilla. That way upstream is made aware of the issue.

    The bug is now being tracked at: https://sourceware.org/bugzilla/show_bug.cgi?id=21716

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Carlos O'Donell@21:1/5 to Johannes Schultz on Wed Jul 5 23:10:02 2017
    XPost: linux.debian.bugs.dist

    On Wed, Jul 5, 2017 at 1:43 PM, Johannes Schultz <[email protected]> wrote:

    None of the internal assertions in tzfile.c have to do with low
    memory, they have to do with logical consistency and expected
    outcomes.

    Okay, so let's look at the stack trace again and where it failed.
    The failing line 779 in __tzfile_compute is:

    if (__tzname[0] == NULL)
    {
    assert (num_types == 1); // <-- boom

    So, where is __tzname[0] being set? Depending on the path taken, it can be either of these:

    line 627: __tzname[0] = NULL; // initial value
    line 646: __tzname[0] = __tzstring (&zone_names[types[i].idx]);
    line 686: __tzname[0] = __tzstring (zone_names);
    line 756: __tzname[dst] = __tzstring (&zone_names[idx]);

    Internally, __tzstring calls malloc. If malloc fails, it returns NULL.
    So it is entirely possible that this assertion will fail because of an out-of-memory situation.

    Perfect, in which case it *should* be fixed to make this more robust in the face of low memory.

    No of course this is bad, but so far the integrity has not been compromised.

    Agreed.

    It just means that the function really should return with an error now. As far as I can see there are currently no facilities for returning an error in this particular function, but I guess it really should be able to propagate allocation failures to the library functions that call it, so that those can return an error to the user (if that is part of their API contract, that
    is).

    I agree.

    If you have the opportunity please file a match bug with upstream at sourceware.org/bugzilla. That way upstream is made aware of the issue.

    As far as I can see we probably need to fix all cases of __tzstring returning NULL and do something appropraite. It only ever returns NULL on ENOMEM.

    Cheers,
    Carlos.

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)