Bug 29030 - GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)
Summary: GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (***...
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: string (show other bugs)
Version: 2.35
: P2 normal
Target Milestone: 2.36
Assignee: Not yet assigned to anyone
URL:
Keywords: glibc_2.35
Depends on:
Blocks:
 
Reported: 2022-04-05 22:00 UTC by Joan Bruguera Micó
Modified: 2022-04-25 16:11 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2022-04-06 00:00:00


Attachments
WIP Patch (1.31 KB, patch)
2022-04-05 22:03 UTC, Joan Bruguera Micó
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joan Bruguera Micó 2022-04-05 22:00:07 UTC
Since the recent update to GLIBC 2.35, I've seen or got reports on Arch Linux, OpenSuse Tumbleweed and Fedora 36 (RPMFusion), of a crash on startup of the popular "moc" (Music on Console) player with the following error message:

    "*** buffer overflow detected ***: terminated"
    Aborted (core dumped)

See e.g. Arch Linux bug report https://bugs.archlinux.org/task/74041

I've been able to reduce the crash down to the following test case:

    #include <wchar.h>

    int main (void)
    {
        const char *hw = "HelloWorld";
        mbsrtowcs (NULL, &hw, (size_t)-1, NULL);
        return 0;
    }

As far as I can tell, this is a valid use of `mbsrtowcs`, since `$2=src` is a valid pointer, `$1=dst` and `$4=ps` are allowed to be NULL, and `$3=len` is ignored when `dst==NULL`.

When built with `gcc -O2 -Wp,-D_FORTIFY_SOURCE=2 test.c -o test && ./test`, however, it crashes with the error message above, plus the following gcc warning:

    In file included from /usr/include/features.h:490,
                     from /usr/include/bits/libc-header-start.h:33,
                     from /usr/include/wchar.h:27,
                     from test.c:1:
    In function 'mbsrtowcs',
        inlined from 'main' at test.c:6:9:
    /usr/include/bits/wchar2.h:428:10: warning: call to '__mbsrtowcs_chk_warn' declared with attribute warning: mbsrtowcs called with dst buffer smaller than len * sizeof (wchar_t) [-Wattribute-warning]
      428 |   return __glibc_fortify_n (mbsrtowcs, __len, sizeof (wchar_t),
          |          ^~~~~~~~~~~~~~~~~

The crash only reproduces with "large enough" values of `$3=len` - note `len` is supposed to be ignored when `dst=NULL` according to the manual. Namely on my x86_64 system it crashes when `len > 0x3FFFFFFFFFFFFFFFULL`.

I've been reviewing recent changes and I believe this regression was introduced by commit a643f60c53876b, which refactors some fortify-related code. I believe the false positive may only trigger on rare edge cases - it needs to be a function where `__glibc_fortify(_n)?` is called with `__s > 1`, and a call with a large buffer size parameter is valid - which as far as I can tell only includes `mbsrtowcs` and similar related functions in wchar2.h. This would explain why this wasn't detected by tests or earlier.
Comment 1 Joan Bruguera Micó 2022-04-05 22:03:39 UTC
Created attachment 14050 [details]
WIP Patch

PS: I have the following WIP patch, I need to finish testing & checking the rules before sending to glibc-alpha.
Comment 2 Siddhesh Poyarekar 2022-04-06 02:24:01 UTC
(In reply to Joan Bruguera Micó from comment #1)
> Created attachment 14050 [details]
> WIP Patch
> 
> PS: I have the following WIP patch, I need to finish testing & checking the
> rules before sending to glibc-alpha.

Thanks, but that's not the correct fix I'm afraid.  The fix is to skip _chk in case dest is NULL in wcsmbs/bits/wchar2.h.  That is, this function:

__fortify_function size_t                                                  
__NTH (mbsrtowcs (wchar_t *__restrict __dst, const char **__restrict __src,
                  size_t __len, mbstate_t *__restrict __ps))               
{                                                                          
  return __glibc_fortify_n (mbsrtowcs, __len, sizeof (wchar_t),            
                            __glibc_objsize (__dst),                       
                            __dst, __src, __len, __ps);                    
}                                                                          

should avoid calling __glibc_fortify_n when __dst is NULL.
Comment 3 Siddhesh Poyarekar 2022-04-06 02:32:19 UTC
Also, please consider fixing mbsnrtowcs along with this since that will also have te same issue.  Thanks!
Comment 4 Siddhesh Poyarekar 2022-04-07 05:14:38 UTC
Thinking about this a bit more, lets go with a variant of your patch, i.e. add the -1 check at the top in __glibc_safe_or_unknown_len.  That should preventively fix any future API (or existing API we may have missed) that has similar semantics to mbsrtowcs.

Please post a patch and cc me, and I'll review it.
Comment 5 cvs-commit@gcc.gnu.org 2022-04-25 12:05:37 UTC
The master branch has been updated by Siddhesh Poyarekar <siddhesh@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=33e03f9cd2be4f2cd62f93fda539cc07d9c8130e

commit 33e03f9cd2be4f2cd62f93fda539cc07d9c8130e
Author: Joan Bruguera <joanbrugueram@gmail.com>
Date:   Mon Apr 11 19:49:56 2022 +0200

    misc: Fix rare fortify crash on wchar funcs. [BZ 29030]
    
    If `__glibc_objsize (__o) == (size_t) -1` (i.e. `__o` is unknown size), fortify
    checks should pass, and `__whatever_alias` should be called.
    
    Previously, `__glibc_objsize (__o) == (size_t) -1` was explicitly checked, but
    on commit a643f60c53876b, this was moved into `__glibc_safe_or_unknown_len`.
    
    A comment says the -1 case should work as: "The -1 check is redundant because
    since it implies that __glibc_safe_len_cond is true.". But this fails when:
    * `__s > 1`
    * `__osz == -1` (i.e. unknown size at compile time)
    * `__l` is big enough
    * `__l * __s <= __osz` can be folded to a constant
    (I only found this to be true for `mbsrtowcs` and other functions in wchar2.h)
    
    In this case `__l * __s <= __osz` is false, and `__whatever_chk_warn` will be
    called by `__glibc_fortify` or `__glibc_fortify_n` and crash the program.
    
    This commit adds the explicit `__osz == -1` check again.
    moc crashes on startup due to this, see: https://bugs.archlinux.org/task/74041
    
    Minimal test case (test.c):
        #include <wchar.h>
    
        int main (void)
        {
            const char *hw = "HelloWorld";
            mbsrtowcs (NULL, &hw, (size_t)-1, NULL);
            return 0;
        }
    
    Build with:
        gcc -O2 -Wp,-D_FORTIFY_SOURCE=2 test.c -o test && ./test
    
    Output:
        *** buffer overflow detected ***: terminated
    
    Fixes: BZ #29030
    Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com>
    Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
Comment 6 Siddhesh Poyarekar 2022-04-25 12:07:02 UTC
I need to backport this and the changes were minor, so I pushed your fix with those changes.  Thank you for your contribution!
Comment 7 cvs-commit@gcc.gnu.org 2022-04-25 13:14:07 UTC
The release/2.35/master branch has been updated by Siddhesh Poyarekar <siddhesh@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=c8ee1c85c07b3c9eaef46355cb1095300855e8fa

commit c8ee1c85c07b3c9eaef46355cb1095300855e8fa
Author: Joan Bruguera <joanbrugueram@gmail.com>
Date:   Mon Apr 11 19:49:56 2022 +0200

    misc: Fix rare fortify crash on wchar funcs. [BZ 29030]
    
    If `__glibc_objsize (__o) == (size_t) -1` (i.e. `__o` is unknown size), fortify
    checks should pass, and `__whatever_alias` should be called.
    
    Previously, `__glibc_objsize (__o) == (size_t) -1` was explicitly checked, but
    on commit a643f60c53876b, this was moved into `__glibc_safe_or_unknown_len`.
    
    A comment says the -1 case should work as: "The -1 check is redundant because
    since it implies that __glibc_safe_len_cond is true.". But this fails when:
    * `__s > 1`
    * `__osz == -1` (i.e. unknown size at compile time)
    * `__l` is big enough
    * `__l * __s <= __osz` can be folded to a constant
    (I only found this to be true for `mbsrtowcs` and other functions in wchar2.h)
    
    In this case `__l * __s <= __osz` is false, and `__whatever_chk_warn` will be
    called by `__glibc_fortify` or `__glibc_fortify_n` and crash the program.
    
    This commit adds the explicit `__osz == -1` check again.
    moc crashes on startup due to this, see: https://bugs.archlinux.org/task/74041
    
    Minimal test case (test.c):
        #include <wchar.h>
    
        int main (void)
        {
            const char *hw = "HelloWorld";
            mbsrtowcs (NULL, &hw, (size_t)-1, NULL);
            return 0;
        }
    
    Build with:
        gcc -O2 -Wp,-D_FORTIFY_SOURCE=2 test.c -o test && ./test
    
    Output:
        *** buffer overflow detected ***: terminated
    
    Fixes: BZ #29030
    Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com>
    Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
    (cherry picked from commit 33e03f9cd2be4f2cd62f93fda539cc07d9c8130e)
Comment 8 cvs-commit@gcc.gnu.org 2022-04-25 16:11:03 UTC
The release/2.34/master branch has been updated by Siddhesh Poyarekar <siddhesh@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=ca0faa140ff8cebe4c041d935f0f5eb480873d99

commit ca0faa140ff8cebe4c041d935f0f5eb480873d99
Author: Joan Bruguera <joanbrugueram@gmail.com>
Date:   Mon Apr 11 19:49:56 2022 +0200

    misc: Fix rare fortify crash on wchar funcs. [BZ 29030]
    
    If `__glibc_objsize (__o) == (size_t) -1` (i.e. `__o` is unknown size), fortify
    checks should pass, and `__whatever_alias` should be called.
    
    Previously, `__glibc_objsize (__o) == (size_t) -1` was explicitly checked, but
    on commit a643f60c53876b, this was moved into `__glibc_safe_or_unknown_len`.
    
    A comment says the -1 case should work as: "The -1 check is redundant because
    since it implies that __glibc_safe_len_cond is true.". But this fails when:
    * `__s > 1`
    * `__osz == -1` (i.e. unknown size at compile time)
    * `__l` is big enough
    * `__l * __s <= __osz` can be folded to a constant
    (I only found this to be true for `mbsrtowcs` and other functions in wchar2.h)
    
    In this case `__l * __s <= __osz` is false, and `__whatever_chk_warn` will be
    called by `__glibc_fortify` or `__glibc_fortify_n` and crash the program.
    
    This commit adds the explicit `__osz == -1` check again.
    moc crashes on startup due to this, see: https://bugs.archlinux.org/task/74041
    
    Minimal test case (test.c):
        #include <wchar.h>
    
        int main (void)
        {
            const char *hw = "HelloWorld";
            mbsrtowcs (NULL, &hw, (size_t)-1, NULL);
            return 0;
        }
    
    Build with:
        gcc -O2 -Wp,-D_FORTIFY_SOURCE=2 test.c -o test && ./test
    
    Output:
        *** buffer overflow detected ***: terminated
    
    Fixes: BZ #29030
    Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com>
    Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
    (cherry picked from commit 33e03f9cd2be4f2cd62f93fda539cc07d9c8130e)