broadcast_coding_history_test : coding_history.wav ...... *** buffer overflow detected ***: /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test terminated ok broadcast_coding_history_size : coding_hist_size.wav .... ======= Backtrace: ========= /lib/libc.so.6(__fortify_fail+0x37)[0x3129ee6a17] /lib/libc.so.6[0x3129ee4830] /lib/libc.so.6[0x3129ee36b7] /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/src/.libs/libsndfile.so.1(+0x3c9bb)[0x2b5e3cf529bb] /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/src/.libs/libsndfile.so.1(sf_command+0x90d)[0x2b5e3cf1d89d] /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test[0x403fd5] /lib/libc.so.6(__libc_start_main+0xfd)[0x3129e1eb6d] /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test[0x4016f9] ======= Memory map: ======== 00400000-0040a000 r-xp 00000000 fe:05 393923 /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test 0060a000-0060b000 r--p 0000a000 fe:05 393923 /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test 0060b000-0060c000 rw-p 0000b000 fe:05 393923 /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test 0060c000-00614000 rw-p 00000000 00:00 0 01b48000-01b69000 rw-p 00000000 00:00 0 [heap] 3129a00000-3129a1e000 r-xp 00000000 08:03 30139 /lib64/ld-2.11.1.so 3129c1d000-3129c1e000 r--p 0001d000 08:03 30139 /lib64/ld-2.11.1.so 3129c1e000-3129c1f000 rw-p 0001e000 08:03 30139 /lib64/ld-2.11.1.so 3129c1f000-3129c20000 rw-p 00000000 00:00 0 3129e00000-3129f55000 r-xp 00000000 08:03 30164 /lib64/libc-2.11.1.so 3129f55000-312a155000 ---p 00155000 08:03 30164 /lib64/libc-2.11.1.so 312a155000-312a159000 r--p 00155000 08:03 30164 /lib64/libc-2.11.1.so 312a159000-312a15a000 rw-p 00159000 08:03 30164 /lib64/libc-2.11.1.so 312a15a000-312a15f000 rw-p 00000000 00:00 0 312a200000-312a281000 r-xp 00000000 08:03 30307 /lib64/libm-2.11.1.so 312a281000-312a480000 ---p 00081000 08:03 30307 /lib64/libm-2.11.1.so 312a480000-312a481000 r--p 00080000 08:03 30307 /lib64/libm-2.11.1.so 312a481000-312a482000 rw-p 00081000 08:03 30307 /lib64/libm-2.11.1.so 312a600000-312a602000 r-xp 00000000 08:03 30189 /lib64/libdl-2.11.1.so 312a602000-312a802000 ---p 00002000 08:03 30189 /lib64/libdl-2.11.1.so 312a802000-312a803000 r--p 00002000 08:03 30189 /lib64/libdl-2.11.1.so 312a803000-312a804000 rw-p 00003000 08:03 30189 /lib64/libdl-2.11.1.so 3135600000-3135605000 r-xp 00000000 fe:00 44198 /usr/lib64/libogg.so.0.7.0 3135605000-3135804000 ---p 00005000 fe:00 44198 /usr/lib64/libogg.so.0.7.0 3135804000-3135805000 r--p 00004000 fe:00 44198 /usr/lib64/libogg.so.0.7.0 3135805000-3135806000 rw-p 00005000 fe:00 44198 /usr/lib64/libogg.so.0.7.0 3135a00000-3135a2a000 r-xp 00000000 fe:00 114319 /usr/lib64/libvorbis.so.0.4.4 3135a2a000-3135c2a000 ---p 0002a000 fe:00 114319 /usr/lib64/libvorbis.so.0.4.4 3135c2a000-3135c2b000 r--p 0002a000 fe:00 114319 /usr/lib64/libvorbis.so.0.4.4 3135c2b000-3135c2c000 rw-p 0002b000 fe:00 114319 /usr/lib64/libvorbis.so.0.4.4 3135e00000-31360b3000 r-xp 00000000 fe:00 79264 /usr/lib64/libvorbisenc.so.2.0.7 31360b3000-31362b2000 ---p 002b3000 fe:00 79264 /usr/lib64/libvorbisenc.so.2.0.7 31362b2000-31362ce000 r--p 002b2000 fe:00 79264 /usr/lib64/libvorbisenc.so.2.0.7 31362ce000-31362cf000 rw-p 002ce000 fe:00 79264 /usr/lib64/libvorbisenc.so.2.0.7 3641400000-3641437000 r-xp 00000000 fe:00 36765 /usr/lib64/libFLAC.so.8.2.0 3641437000-3641636000 ---p 00037000 fe:00 36765 /usr/lib64/libFLAC.so.8.2.0 3641636000-3641637000 r--p 00036000 fe:00 36765 /usr/lib64/libFLAC.so.8.2.0 3641637000-3641638000 rw-p 00037000 fe:00 36765 /usr/lib64/libFLAC.so.8.2.0 3d55a00000-3d55a15000 r-xp 00000000 08:03 30146 /lib64/libgcc_s.so.1 3d55a15000-3d55c14000 ---p 00015000 08:03 30146 /lib64/libgcc_s.so.1 3d55c14000-3d55c15000 r--p 00014000 08:03 30146 /lib64/libgcc_s.so.1 3d55c15000-3d55c16000 rw-p 00015000 08:03 30146 /lib64/libgcc_s.so.1 2b5e3cd02000-2b5e3cd03000 rw-p 00000000 00:00 0 2b5e3cd03000-2b5e3cd11000 r-xp 00000000 fe:00 33368 /usr/lib64/libsandbox.so 2b5e3cd11000-2b5e3cf11000 ---p 0000e000 fe:00 33368 /usr/lib64/libsandbox.so 2b5e3cf11000-2b5e3cf12000 r--p 0000e000 fe:00 33368 /usr/lib64/libsandbox.so 2b5e3cf12000-2b5e3cf13000 rw-p 0000f000 fe:00 33368 /usr/lib64/libsandbox.so 2b5e3cf13000-2b5e3cf16000 rw-p 00000000 00:00 0 2b5e3cf16000-2b5e3cf76000 r-xp 00000000 fe:05 393835 /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/src/.libs/libsndfile.so.1.0.21 2b5e3cf76000-2b5e3d175000 ---p 00060000 fe:05 393835 /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/src/.libs/libsndfile.so.1.0.21 2b5e3d175000-2b5e3d177000 r--p 0005f000 fe:05 393835 /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/src/.libs/libsndfile.so.1.0.21 2b5e3d177000-2b5e3d178000 rw-p 00061000 fe:05 393835 /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/src/.libs/libsndfile.so.1.0.21 2b5e3d178000-2b5e3d1a9000 rw-p 00000000 00:00 0 2b5e3d1a9000-2b5e3d1f2000 rw-p 00000000 00:00 0 7fffcfc03000-7fffcfc1b000 rw-p 00000000 00:00 0 [stack] 7fffcfdcd000-7fffcfdce000 r-xp 00000000 00:00 0 [vdso] ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall] test_wrapper.sh: line 60: 25495 Aborted ./command_test bextch make[1]: *** [check] Error 134 make: *** [check-recursive] Error 1 * ERROR: media-libs/libsndfile-1.0.21 failed
From libsndfile author: With gcc-4.5 from Debian's experimental distro on x86_64 I did: CC=gcc-4.5 CPPFLAGS="-D_FORTIFY_SOURCE=2" ./configure --disable-shared make clean make check which reproduced this problem. I then did: CC=gcc-4.5 CPPFLAGS="-D_FORTIFY_SOURCE=2" ./configure --disable-shared --disable-gcc-opt make clean make check which passed all tests. The difference between the two is that the first compiles with optimisation flag of -O2 while the second is compiled with -O0 (ie no optimisation). Since the failure only occurs with optimisation on, I believe that this is actually a bug in gcc. Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo
Hahahahaha. Yeah sure, sure, it's always a miscompilation problem when upstream doesn't feel like running gdb to see what crashed.
Ryan, could you rebuild libsndfile with debugging symbols and post a backtrace from that? Based on the response cited in comment #1, it sounds like this will need to be fixed in Gentoo and pushed upstream. Since this only recently appeared, I suspect that gcc 4.5 is a prerequisite for whatever check triggered. Having a backtrace may help users who still run an older gcc locate the offending code for analysis and repair, thereby freeing gcc-4.5 testers to work on other things.
halo ~/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs # gdb ./command_test GNU gdb (Gentoo 7.1 p1) 7.1 Copyright (C) 2010 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-unknown-linux-gnu". For bug reporting instructions, please see: <http://bugs.gentoo.org/>... Reading symbols from /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test...done. (gdb) run bextch Starting program: /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test bextch warning: no loadable sections found in added symbol-file /usr/lib64/debug/lib64/ld-2.11.1.so.debug warning: no loadable sections found in added symbol-file /usr/lib64/debug/usr/lib64/libsndfile.so.1.0.21.debug warning: no loadable sections found in added symbol-file /usr/lib64/debug/usr/lib64/libFLAC.so.8.2.0.debug warning: no loadable sections found in added symbol-file /usr/lib64/debug/usr/lib64/libvorbisenc.so.2.0.7.debug warning: no loadable sections found in added symbol-file /usr/lib64/debug/usr/lib64/libvorbis.so.0.4.4.debug warning: no loadable sections found in added symbol-file /usr/lib64/debug/usr/lib64/libogg.so.0.7.0.debug broadcast_coding_history_test : coding_history.wav ...... ok broadcast_coding_history_size : coding_hist_size.wav .... *** buffer overflow detected ***: /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test terminated ======= Backtrace: ========= /lib/libc.so.6(__fortify_fail+0x37)[0x33e94e6a17] /lib/libc.so.6[0x33e94e4830] /lib/libc.so.6[0x33e94e36b7] /usr/lib/libsndfile.so.1(+0x3cded)[0x7ffff7da4ded] /usr/lib/libsndfile.so.1(sf_command+0x907)[0x7ffff7d6f7d7] /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test[0x404912] /lib/libc.so.6(__libc_start_main+0xfd)[0x33e941eb6d] /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test[0x4016a9] ======= Memory map: ======== 00400000-0040b000 r-xp 00000000 fe:05 393922 /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test 0060a000-0060b000 r--p 0000a000 fe:05 393922 /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test 0060b000-0060c000 rw-p 0000b000 fe:05 393922 /var/tmp/portage/media-libs/libsndfile-1.0.21/work/libsndfile-1.0.21/tests/.libs/command_test 0060c000-00635000 rw-p 00000000 00:00 0 [heap] 33e7e00000-33e7e1e000 r-xp 00000000 08:03 29683 /lib64/ld-2.11.1.so 33e801d000-33e801e000 r--p 0001d000 08:03 29683 /lib64/ld-2.11.1.so 33e801e000-33e801f000 rw-p 0001e000 08:03 29683 /lib64/ld-2.11.1.so 33e801f000-33e8020000 rw-p 00000000 00:00 0 33e9400000-33e9555000 r-xp 00000000 08:03 29699 /lib64/libc-2.11.1.so 33e9555000-33e9755000 ---p 00155000 08:03 29699 /lib64/libc-2.11.1.so 33e9755000-33e9759000 r--p 00155000 08:03 29699 /lib64/libc-2.11.1.so 33e9759000-33e975a000 rw-p 00159000 08:03 29699 /lib64/libc-2.11.1.so 33e975a000-33e975f000 rw-p 00000000 00:00 0 33e9800000-33e9881000 r-xp 00000000 08:03 29958 /lib64/libm-2.11.1.so 33e9881000-33e9a80000 ---p 00081000 08:03 29958 /lib64/libm-2.11.1.so 33e9a80000-33e9a81000 r--p 00080000 08:03 29958 /lib64/libm-2.11.1.so 33e9a81000-33e9a82000 rw-p 00081000 08:03 29958 /lib64/libm-2.11.1.so 3ad0c00000-3ad0c05000 r-xp 00000000 fe:00 37744 /usr/lib64/libogg.so.0.7.0 3ad0c05000-3ad0e04000 ---p 00005000 fe:00 37744 /usr/lib64/libogg.so.0.7.0 3ad0e04000-3ad0e05000 r--p 00004000 fe:00 37744 /usr/lib64/libogg.so.0.7.0 3ad0e05000-3ad0e06000 rw-p 00005000 fe:00 37744 /usr/lib64/libogg.so.0.7.0 3ad1000000-3ad102a000 r-xp 00000000 fe:00 41603 /usr/lib64/libvorbis.so.0.4.4 3ad102a000-3ad122a000 ---p 0002a000 fe:00 41603 /usr/lib64/libvorbis.so.0.4.4 3ad122a000-3ad122b000 r--p 0002a000 fe:00 41603 /usr/lib64/libvorbis.so.0.4.4 3ad122b000-3ad122c000 rw-p 0002b000 fe:00 41603 /usr/lib64/libvorbis.so.0.4.4 3ad1400000-3ad1437000 r-xp 00000000 fe:00 42287 /usr/lib64/libFLAC.so.8.2.0 3ad1437000-3ad1636000 ---p 00037000 fe:00 42287 /usr/lib64/libFLAC.so.8.2.0 3ad1636000-3ad1637000 r--p 00036000 fe:00 42287 /usr/lib64/libFLAC.so.8.2.0 3ad1637000-3ad1638000 rw-p 00037000 fe:00 42287 /usr/lib64/libFLAC.so.8.2.0 3ad1800000-3ad1ab3000 r-xp 00000000 fe:00 52613 /usr/lib64/libvorbisenc.so.2.0.7 3ad1ab3000-3ad1cb2000 ---p 002b3000 fe:00 52613 /usr/lib64/libvorbisenc.so.2.0.7 3ad1cb2000-3ad1cce000 r--p 002b2000 fe:00 52613 /usr/lib64/libvorbisenc.so.2.0.7 3ad1cce000-3ad1ccf000 rw-p 002ce000 fe:00 52613 /usr/lib64/libvorbisenc.so.2.0.7 3f52a00000-3f52a15000 r-xp 00000000 08:03 29595 /lib64/libgcc_s.so.1 3f52a15000-3f52c14000 ---p 00015000 08:03 29595 /lib64/libgcc_s.so.1 3f52c14000-3f52c15000 r--p 00014000 08:03 29595 /lib64/libgcc_s.so.1 3f52c15000-3f52c16000 rw-p 00015000 08:03 29595 /lib64/libgcc_s.so.1 7ffff7d63000-7ffff7d68000 rw-p 00000000 00:00 0 7ffff7d68000-7ffff7dc8000 r-xp 00000000 fe:00 22740 /usr/lib64/libsndfile.so.1.0.21 7ffff7dc8000-7ffff7fc8000 ---p 00060000 fe:00 22740 /usr/lib64/libsndfile.so.1.0.21 7ffff7fc8000-7ffff7fca000 r--p 00060000 fe:00 22740 /usr/lib64/libsndfile.so.1.0.21 7ffff7fca000-7ffff7fcb000 rw-p 00062000 fe:00 22740 /usr/lib64/libsndfile.so.1.0.21 7ffff7fcb000-7ffff7fcf000 rw-p 00000000 00:00 0 7ffff7ffc000-7ffff7ffe000 rw-p 00000000 00:00 0 7ffff7ffe000-7ffff7fff000 r-xp 00000000 00:00 0 [vdso] 7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0 [stack] ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall] Program received signal SIGABRT, Aborted. 0x00000033e9432545 in raise () from /lib/libc.so.6 (gdb) bt full #0 0x00000033e9432545 in raise () from /lib/libc.so.6 No symbol table info available. #1 0x00000033e94339c6 in abort () from /lib/libc.so.6 No symbol table info available. #2 0x00000033e946d033 in __libc_message () from /lib/libc.so.6 No symbol table info available. #3 0x00000033e94e6a17 in __fortify_fail () from /lib/libc.so.6 No symbol table info available. #4 0x00000033e94e4830 in __chk_fail () from /lib/libc.so.6 No symbol table info available. #5 0x00000033e94e36b7 in __strcat_chk () from /lib/libc.so.6 No symbol table info available. #6 0x00007ffff7da4ded in broadcast_var_set (psf=0x614010, info=<value optimized out>, datasize=<value optimized out>) at /usr/include/bits/string3.h:154 added_history = "A=PCM,F=22050,W=16,M=mono,T=libsndfile-1.0.21\r\n\000\000\000\000\000\000\000\000\000\000\201\000\000\000\000\000\000\030\000\000\000\000\000\000\000p\335\377\377\377\177\000\000\000\335\377\377\377\177\000\000\000\020", '\000' <repeats 14 times>"\325, %\bL\000\000\000\000\236\344<-\000\000\000\000\306(\bL", '\000' <repeats 12 times>, "d.L\351\063\000\000\000\001", '\000' <repeats 23 times>, "\020@a\000\000\000\000\000 \336\377\377\377\177\000\000\230%b", '\000' <repeats 14 times>"\250, \374\367\377\177\000\000\350\337\377\377\377\177\000\000\034\060\327\367\377\177\000\000f\340\243-\000\000\000\000R:\331\367\377\177\000\000\000\000\000\000\000\000\000\000\005\000\000\000\000\000\000" added_history_len = 0 len = <value optimized out> #7 0x00007ffff7d6f7d7 in sf_command (sndfile=0x614010, command=<value optimized out>, data=0x610de0, datasize=<value optimized out>) at sndfile.c:1150 psf = 0x614010 ---Type <return> to continue, or q <return> to quit--- old_value = <value optimized out> #8 0x0000000000404912 in broadcast_coding_history_size (argc=<value optimized out>, argv=0x7fffffffdfe8) at command_test.c:1184 file = 0x614010 k = <value optimized out> sfinfo = {frames = 0, samplerate = 22050, channels = 1, format = 65538, sections = 1, seekable = 1} bc_write = {description = "Test description", '\000' <repeats 239 times>, originator = "Test originator", '\000' <repeats 16 times>, originator_reference = "4c0828c6-b3f7d739", '\000' <repeats 14 times>, origination_date = "2006/03/3", origination_time = "20:27:0", time_reference_low = 0, time_reference_high = 0, version = 0, umid = "Some umid", '\000' <repeats 54 times>, reserved = '\000' <repeats 189 times>, coding_history_size = 520, coding_history = "line 0\nline 1\nline 2\nline 3\nline 4\nline 5\nline 6\nline 7\nline 8\nline 9\nline 10\nline 11\nline 12\nline 13\nline 14\nline 15\nline 16\nline 17\nline 18\nline 19\n"...} bc_read = {description = '\000' <repeats 255 times>, originator = '\000' <repeats 31 times>, originator_reference = '\000' <repeats 31 times>, origination_date = "\000\000\000\000\000\000\000\000\000", origination_time = "\000\000\000\000\000\000\000", time_reference_low = 0, time_reference_high = 0, version = 0, umid = '\000' <repeats 63 times>, reserved = '\000' <repeats 189 times>, coding_history_size = 0, coding_history = '\000' <repeats 1023 times>} #9 main (argc=<value optimized out>, argv=0x7fffffffdfe8) at command_test.c:152 do_all = <value optimized out> test_count = <value optimized out>
The optimizations slightly obscure the exact culprit, but the existing code is definitely incorrect in lines 92-96. Among other things, it uses strncat in such a way that you may as well just call strcat - it is unsafe either way, and calling strcat would be less typing. It needs to be changed to instead pass sizeof(psf->broadcast_var->binfo.coding_history) - strlen(psf->broadcast_var->binfo.coding_history) - 1, so that strncat cannot overrun the buffer. The helper function strncpy_crlf also has some edge cases that could cause a buffer overread and/or buffer overrun write, but I think they would do so in such a way that the _FORTIFY_SOURCE checks would not detect it, since they operate on direct pointer writes instead of through instrumented glibc calls.
From upstream libsndfile maintainer: "Last time I looked at this I spent a number of hours compiling, testing, running under gdb and valgrind and adding debug printf statements. The result of that investigation led me to the conclusion that this was a tool chain error. After being bugged about this some more I revisited the problem. I basically redid all my previous investigation again, from scratch. I also researched _FORTIFY_SOURCE on the net. The results of this investigation reinforced my opinion that this was a tool chain issue of some sort. After sleeping on the issue I tried one last thing, setting both CPPFLAGS and CFLAGS. Interestingly, setting both variables resulted in a passing test. In detail this (in Debian experimental chroot) will fail: CC=gcc-4.5 CPPFLAGS="-D_FORTIFY_SOURCE=2" ./configure --disable-shared make clean make tests/command_test bextch while this (same environment) will pass: CC=gcc-4.5 CFLAGS="-D_FORTIFY_SOURCE=2" \ CPPFLAGS="-D_FORTIFY_SOURCE=2" ./configure --disable-shared make clean make tests/command_test bextch While not actually an issue with the toolchain, this was an issue with the use of the toolchain and not a problem with libsndfile."
Created attachment 243659 [details, diff] Debugging patch for libsndfile-1.0.21.ebuild -- no actual code fix (In reply to comment #6) > From upstream libsndfile maintainer: > > "While not actually an issue with the toolchain, this was an issue > with the use of the toolchain and not a problem with libsndfile." While it is certainly possible that libsndfile is invoking the toolchain in some strange way that necessitates specifying fortification in both CPPFLAGS and CFLAGS, that is not normally required. A quick check of a libsndfile build log with V=1 shows nothing obviously wrong with its invocation of gcc. Additionally, I would be more concerned about the issues cited in comment #5, since distributions will keep pushing automated checks and the code I examined definitely has a problem with those checks. Rather than trying to prove that by code inspection, I applied a small change to the ebuild so that src/broadcast.c!broadcast_set_var would be marked up with asserts to catch overruns earlier (see attached). With this check added, the test program now dies of an assertion failure instead of a buffer overrun check. Examining the surrounding variables shows that the caller provided an info->coding_history that was too long, and strncpy_crlf blindly copied past the end of psf->broadcast_var->binfo.coding_history. Inspection of the surrounding area suggests that the author is doing something strange with dynamically allocating a variable sized memory block for psf->broadcast_var, so there might not be an actual memory corruption bug here, at least for the test case that spawned this bug report. However, as written, the code definitely exceeds the _declared_ dimensions of the array, so the fortification checks step in to prevent potential memory corruption. As a side note, bad behavior by FLAC disrupts use of this patch. FLAC installs its own "assert.h" in /usr/include/FLAC, then arranges for 'pkg-config --cflags flac' to print -I/usr/include/FLAC, causing "#include <assert.h>" to grab the FLAC assert instead of the real one. To use the attached patch, temporarily rename/remove the disruptive FLAC assert.h. That needs to be fixed properly elsewhere.
I'm stopping doing forwarder job with this latest reply from upstream maintainer: -----------8<-----------8<-----------8<-----------8<-----------8<----------- > While it is certainly possible that libsndfile is invoking the toolchain in > some strange way that necessitates specifying fortification in both CPPFLAGS > and CFLAGS, that is not normally required. A quick check of a libsndfile build > log with V=1 shows nothing obviously wrong with its invocation of gcc. I actually suspect its a problem with libtool. > Additionally, I would be more concerned about the issues cited in comment #5, If you were right Valgrind (a tool which I use all the time) would complain. Valgrind doesn't complain hence this is not the issue you think it is. Unfortunately I am not willing to respond to libsndfile bug reports in the bug tracker for any one of the hundreds of Linux distributions that I don't personally use. Let me make this clear. I would vastly prefer not to have to return to this bugtracker about this issue. If you wish to pursue this issue you can join the libsndfile-devel mailing list and discuss it there. -----------8<-----------8<-----------8<-----------8<-----------8<-----------
(In reply to comment #8) > I'm stopping doing forwarder job with this latest reply Thank you for handling forwarding so far. It has been very convenient keeping all the data in one place (this Gentoo bug), rather than spreading it between this and the upstream mailing list, which (at least at first glance) is not readable to non-subscribers. Based on upstream responses so far, and his misunderstanding detailed below, it seems likely that this issue will need to be fixed in Gentoo and then pushed upstream. To that end, I have enclosed some additional commentary for the benefit of whoever reads this bug and undertakes to provide a fix. > > Additionally, I would be more concerned about the issues cited in comment #5, > > If you were right Valgrind (a tool which I use all the time) would complain. > Valgrind doesn't complain hence this is not the issue you think it is. Upstream is correct that Valgrind will report memory errors, _but_ he misunderstands or ignores that it can only report an error when given a workload which induces memory corruption. The problem reported in comment #0 and also demonstrated by the assertions added via attachment #243659 [details, diff] is that the data exceeds the _declared_ bounds of the array, not the allocated bounds. As such, Valgrind reports no issue because no corruption has yet occurred and Valgrind is privy to the true size of the allocation, since it monitors malloc calls. However, the code in question can exceed the declared bounds. Any attempt to exceed those bounds triggers a fortification abort, since the fortification can see declared bounds, but cannot see the allocated bounds. Beyond that problem, I asserted (and have seen no challenges) that strncpy_crlf can exceed not only the declared bounds, but also the allocated bounds by way of its feature to convert a bare LF into a CR-LF pair. So there are two problems to fix. At a minimum, we need to fix strncpy_crlf so that it can never exceed the bounds specified by its caller. Additionally, before this package can be used with a fortification-enabled >=gcc-4.5, we need to do something about the use of both strncpy_crlf and CRT string functions to write past the declared array bounds. There are a few options for that: (1) Disable fortification for this file and possibly others. I dislike this idea on principle, since it suppresses a security check and this file is known to have at least one buffer overwrite already, so I am reluctant to trust that there are no bugs that would be made exploitable by removal of fortification. (2) Modify the code to try to prevent gcc from recognizing that it knows the declared size of the buffer, thereby suppressing use of the __builtin_object_size strncat variant that is aborting the program. (3) Modify the code not to rely on the pattern of allocating slack space beyond the structure, so that instead the structure allocates a separate buffer of sufficient size. Option 2 will probably not be too invasive, but may break later if the gcc developers improve their code analysis phases to see past whatever obfuscation is employed here. Option 3 will be resilient long term, but may be considerably more invasive, which could make upstream reluctant to accept the patch.
The only option worth exercising is the one that fixes the code. If upstream is too stubborn/clueless to accept it then that's their problem. Let their wakeup call come with a CVE number attached.
(In reply to comment #10) > The only option worth exercising is the one that fixes the code. The code has not yet been shown to be wrong. As Kevin P says in comment #9: > > is that > > the data exceeds the _declared_ bounds of the array, not the allocated > > bounds. (In reply to comment #10) > If upstream > is too stubborn/clueless to accept it then that's their problem. Nice way to build a relationship with upstream.
(In reply to comment #9) > Upstream is correct that Valgrind will report memory errors, _but_ he > misunderstands or ignores that it can only report an error when given a > workload which induces memory corruption. The problem reported in comment #0 > and also demonstrated by the assertions added via attachment #243659 [details, diff] is that > the data exceeds the _declared_ bounds of the array, not the allocated bounds. Yes, but the allocated bounds are guaranteed > declared bounds. > Beyond that problem, I asserted (and have seen no challenges) that strncpy_crlf > can exceed not only the declared bounds, but also the allocated bounds by way > of its feature to convert a bare LF into a CR-LF pair. If there is a problem as you suggest, it would be simple to write code that uses sf_command() API call correctly and causes strncpy_crlf() to overwrite the bounds of the _allocated_ data chunk. Have you done that?
(In reply to comment #9) > (1) Disable fortification for this file and possibly others. I dislike this > idea on principle, since it suppresses a security check and this file is known > to have at least one buffer overwrite already, so I am reluctant to trust that > there are no bugs that would be made exploitable by removal of fortification. Well how about enabling fortification by setting both CFLAGS and CPPFLAGS?
Created attachment 244067 [details] Test program around strncpy_crlf to demonstrate that chosen inputs can cause it to overrun the output buffer (In reply to comment #12) > Yes, but the allocated bounds are guaranteed > declared bounds. The compiler will not see that because that statement holds only so long as psf->broadcast_var->binfo.coding_history_size has a value that ensures necessary reallocations of the structure occur. Therefore, it takes the much more conservative approach of assuming that the array is as big as you declared it to be, and no bigger. We can agree that the code is intended to allocate a bigger buffer, but the compiler cannot prove that it will. From the compiler's perspective, binfo.coding_history_size might well hold 2**24 on entry, with an actual allocated size of exactly the declared dimensions (plus your 512 bytes of slack) for psf->broadcast_var. Such an arrangement would be stupid and arguably a violation of sane usage, but nothing visible to the compiler proclaims that this will not happen. Thus, we come to the problem described at the end of comment #9: disable fortification or change the allocation pattern to discourage the compiler from thinking it knows the size of the allocation. > > Beyond that problem, I asserted (and have seen no challenges) that strncpy_crlf > > can exceed not only the declared bounds, but also the allocated bounds by way > > of its feature to convert a bare LF into a CR-LF pair. > > If there is a problem as you suggest, it would be simple to write code that > uses sf_command() API call correctly and causes strncpy_crlf() to overwrite the > bounds of the _allocated_ data chunk. Have you done that? I have not drafted a full program around use of the libsndfile API, but I did copy strncpy_crlf to a freestanding source file and call it with chosen inputs that led to memory corruption. See attached. From inspection of broadcast_var_set, I believe a caller of the libsndfile API could induce the same corruption by setting a "normal" string for coding_history, then setting a string consisting entirely of newlines, with the length chosen to be equal to psf->broadcast_var->binfo.coding_history_size + added_history_len, so that the reallocation check is not triggered. The caller might also be able to leverage the rounding applied on line 101 to confuse a subsequent call into thinking that one more byte is available than is the case. However, I have not investigated this path. (In reply to comment #13) > Well how about enabling fortification by setting both CFLAGS and CPPFLAGS? Hardened GCC sets fortification via a modified gcc specs, bypassing the build system entirely. Based on the text of the shipped configure script, line 4067 and lines 4132-4146, setting CFLAGS explicitly in the configure environment seems to suppress the default of "-g -O2" (or "-O2" if -g is not accepted), so your successful build description relayed in comment #6 is fundamentally the same as the build description relayed in comment #1. The check in question requires the compiler's optimization phase to work, so the successful run in those cases is expected. Analysis of the assembly should confirm that the checked variants were not called. Also, lost in all this argument about whether the allocations are big enough is that the usage of strncat on line 93 and line 96 is wrong. The third argument should be a bound on the destination, not the source.
>=1.0.22 no longer triggers this.
(In reply to comment #15) > >=1.0.22 no longer triggers this. I see that strncpy_crlf also gained some tests and had its upper bound adjusted. I will take your word on it that gcc-4.5 no longer generates a fortification abort on the included test case, which was the original purpose of this bug. This was probably fixed under this changelog bullet: Fix a bunch of minor issues found using static analysis. However, the strncat usage in src/broadcast.c!broadcast_var_set is still bogus, as described in comment #5. The affected function passes the sizeof the first buffer to strncat, rather than the count of remaining bytes, as described by man strncat: If src contains n or more characters, strncat() writes n+1 characters to dest (n from src plus the terminating null byte). Therefore, the size of dest must be at least strlen(dest)+n+1. I presume fixing that issue would be off-topic here. Would you like to track it in a new Gentoo bug or try to get that fixed upstream and then backported? I think this remaining issue is less likely to be a potential security problem, but have not done any analysis to confirm this.
