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.
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.
(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.
Also, please consider fixing mbsnrtowcs along with this since that will also have te same issue. Thanks!
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.
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>
I need to backport this and the changes were minor, so I pushed your fix with those changes. Thank you for your contribution!
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)
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)