Hi Martin, On Tue, Feb 13, 2018 at 4:05 AM, Martin Storsjö <[email protected]> wrote: > On Tue, 13 Feb 2018, Sean McGovern wrote: > >> Using strcmp() with constant arrays in recent versions of GCC, >> the compiler will "optimize" the calls to use memcmp() instead. >> >> This can be problematic as some implementations of memcmp() are written >> to compare full words at a time which can cause an out-of-bounds read. >> >> Avoid the invalid read by using strncmp() instead. >> --- >> libavformat/network.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/network.c b/libavformat/network.c >> index 86d7955..2bbbb93 100644 >> --- a/libavformat/network.c >> +++ b/libavformat/network.c >> @@ -252,7 +252,7 @@ static int match_host_pattern(const char *pattern, >> const char *hostname) >> if (len_p > len_h) >> return 0; >> // Simply check if the end of hostname is equal to 'pattern' >> - if (!strcmp(pattern, &hostname[len_h - len_p])) { >> + if (!strncmp(pattern, &hostname[len_h - len_p], len_h)) { >> if (len_h == len_p) >> return 1; // Exact match >> if (hostname[len_h - len_p - 1] == '.') >> -- >> 2.7.4 > > > Despite the commit message, I don't really understand what's happening. Can > you give a more detailed explanation? I don't want to obfuscate code to > dance around optimizations unless I at least understand why and how. > > // Martin >
I discovered this while doing a full valgrind FATE run on a POWER7 machine -- among others, fate-noproxy failed. The result for the noproxy test in this case makes me believe it is using the aforementioned behaviour of memcmp(): ==47650== Memcheck, a memory error detector ==47650== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al. ==47650== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info ==47650== Command: /home/seanmcg/build/libav-gcc7/libavformat/tests/noproxy ==47650== ==47650== Invalid read of size 8 ==47650== at 0x1000646C: match_host_pattern (network.c:255) ==47650== by 0x1000646C: ff_http_match_no_proxy (network.c:284) ==47650== by 0x100059CF: test (noproxy.c:25) ==47650== by 0x100057BB: main (noproxy.c:34) ==47650== Address 0x4480054 is 20 bytes inside a block of size 23 alloc'd ==47650== at 0x40861E4: malloc (vg_replace_malloc.c:298) ==47650== by 0x4088AD3: realloc (vg_replace_malloc.c:785) ==47650== by 0x100A9A2F: av_realloc (mem.c:116) ==47650== by 0x100A9A2F: av_strdup (mem.c:215) ==47650== by 0x10006267: ff_http_match_no_proxy (network.c:272) ==47650== by 0x100059CF: test (noproxy.c:25) ==47650== by 0x100057BB: main (noproxy.c:34) ==47650== <..snipped for brevity, pattern repeats for each test..> I found the following bug reports which seemed relevant in the GCC Bugzilla: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52171 and https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78257 I don't think this is a compiler bug or cargo-culting, although Clang does not appear to exhibit this behaviour. -- Sean McG. _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
