Hi Martin,

On Tue, Feb 13, 2018 at 4:05 AM, Martin Storsjö <mar...@martin.st> 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
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to