[FFmpeg-devel] [PATCH] matroskadec: fix NULL pointer dereference

[James Almer]

On 10/17/2016 4:47 AM, Benoit Fouet wrote:
> Hi,
> 
> On 17/10/2016 06:49, James Almer wrote:
>> On 10/16/2016 9:30 PM, James Almer wrote:
>>> On 10/16/2016 5:11 PM, Andreas Cadhalpun wrote:
>>>> The problem was introduced in commit 1273bc6.
>>>>
>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>>>> ---
>>>>   libavformat/matroskadec.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>>> index 8847c62..a5d3c0e 100644
>>>> --- a/libavformat/matroskadec.c
>>>> +++ b/libavformat/matroskadec.c
>>>> @@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext *matroska, int64_t field_order)
>>>>         /* workaround a bug in our Matroska muxer, introduced in version 57.36 alongside
>>>>        * this function, and fixed in 57.52 */
>>>> -    if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, &micro) == 3)
>>>> +    if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, &micro) == 3)
>>> LGTM.
>>>
>>> Matroska files are supposed to always have that element, but even ffmpeg used
>>> to mux files without it at some point when bitexact flag was enabled, so i
>>> guess plenty of files out there are missing it.
>>>
>>>>           bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100);
>>>>         switch (field_order) {
>>>>
>> Just tried a file missing the muxingapp element, meaning matroska->muxingapp
>> is NULL, and sscanf simply returns -1 and sets errno to EINVAL.
>>
>> Where does it crash for you and using what file?
> 
> FWIW, I've just written a quick test on my machine, and an "sscanf(NULL, "%d", &i)" segfaults.

Guess it's up to the implementation, then. I tried with mingw-w64 only.
The documentation for sscanf says it sets errno to EINVAL if *format is NULL,
but doesn't mention *str.

Patch still LGTM. It doesn't hurt either way.