[FFmpeg-devel] [PATCH]lavc/svq3: Do not write into const memory

[Carl Eugen Hoyos]

2018-01-21 16:10 GMT+01:00 Tomas Härdin <tjoppen at acc.umu.se>:
> fre 2018-01-19 klockan 19:19 +0100 skrev Carl Eugen Hoyos:
>> 2018-01-19 16:54 GMT+01:00 Tomas Härdin <tjoppen at acc.umu.se>:
>> > On 2018-01-18 23:34, Carl Eugen Hoyos wrote:
>> > >
>> > > Hi!
>> > >
>> > > Attached patch fixes a warning, I suspect it makes the code more
>> > > correct.
>> > >
>> > > Please comment, Carl Eugen
>> > >
>> > > --- a/libavcodec/svq3.c
>> > > +++ b/libavcodec/svq3.c
>> > > @@ -1048,12 +1048,12 @@ static int
>> > > svq3_decode_slice_header(AVCodecContext
>> > > *avctx)
>> > >           }
>> > >           memcpy(s->slice_buf, s->gb.buffer + s->gb.index / 8,
>> > > slice_bytes);
>> > >   -        init_get_bits(&s->gb_slice, s->slice_buf, slice_bits);
>> >
>> > Unrelated change?
>>
>> No, it is an intended move.
>>
>> > >           if (s->watermark_key) {
>> > > -            uint32_t header = AV_RL32(&s->gb_slice.buffer[1]);
>> > > -            AV_WL32(&s->gb_slice.buffer[1], header ^ s-
>> > > >watermark_key);
>> >
>> > Strange that this didn't manage to break anything, or that the
>> > compiler
>> > let it through
>>
>> A compiler's warning that gets fixed is the reason for this patch but
>> I apparently misunderstand your comment.
>>
>> > > +            uint32_t header = AV_RL32(&s->slice_buf[1]);
>> > > +            AV_WL32(&s->slice_buf[1], header ^ s-
>> > > >watermark_key);
>> >
>> > Considering the memcpy() above, either this or the old code must be
>> > wrong.
>>
>> Why do you think so?
>> (I do consider the old code "wrong", that is why I sent this patch.)
>>
>> > My guess is the old code must have been wrong, since to fiddle
>> > the same bits this AV_WL32() would need to set
>> > &s->slice_buf[1 - s->gb.index / 8]...
>>
>> I was hoping that the same bits get fiddled given that I call the
>> same macro / function...
>
> No I was confusing myself. Your patch looks fine. With these changes
> slice_buf is twiddled into the right state *before* init_get_bits(),
> which makes sense.

Patch applied, thank you!

Carl Eugen