[FFmpeg-devel] Patch (1/2) Decoding of Teletext Descriptor (0x56)

[Clément Bœsch]

On Mon, Sep 23, 2013 at 01:32:16PM +0100, JULIAN GARDNER wrote:
> 
> 
> 
> 
> ----- Original Message -----
> > From: Clément Bœsch <u at pkh.me>
> > To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> > Cc: 
> > Sent: Monday, 23 September 2013, 14:28
> > Subject: Re: [FFmpeg-devel] Patch (1/2) Decoding of Teletext Descriptor (0x56)
> > 
> > On Mon, Sep 23, 2013 at 01:25:19PM +0100, JULIAN GARDNER wrote:
> > [...]
> >>  >> +            if (l<(sizeof( language) - 9)) {
> >>  >> +                mag = type;
> >>  >> +                type >>= 3;
> >>  >> +                if (type < sizeof( types)) {
> >>  >> +                    mag &= 7; if (!mag) mag = 8;
> >>  >> +                    sprintf( language+l, 
> > "%c%c%c,%c%d%02x,", l0, l1,
> >>  >> +                        l2, types[ type], mag, page);
> >>  >
> >>  >use snprintf, the your if don't look safe enougth to me.
> >>  >
> >> 
> >> 
> >>  > +            if (l<(sizeof( language) - 9)) {
> >>  Hmm this protects against a buffer overrun
> >> 
> > 
> > At least %d can be abused in your sprintf. Maybe %02x as well depending on
> > the situation. Just use the safe snprintf.
> > 
> 
> mag can be 1-8
> page can be 0-255, 00 to FF
> 
> How can this be abused, they are fixed sizes in the stream, 1 byte each?
> 

Right now there is no issue, but that code might change and it's not good
practice. Now if you want real examples of broken cases, there might be an
issue with your code: sprintf will write a \0 at the end of the string in
position=9 (10th char.). So you unless I'm missing something, you actually
have a bug. I suggest you fix your code by using a snprintf instead of
trying to make smart ifs and add manual constraints in your code.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130923/c18c27eb/attachment.asc>