[FFmpeg-devel] [PATCH] avformat/pjsdec: check strcspn values before using them

[Michael Niedermayer]

On Fri, Jan 10, 2014 at 11:48:20PM +0100, Clément Bœsch wrote:
> On Fri, Jan 10, 2014 at 02:05:47AM +0100, Michael Niedermayer wrote:
> > Fixes use of uninitialized memory
> > Fixes: msan_uninit-mem_7f91f2de7764_2649_PJS_capability_tester.pjs
> > Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > ---
> >  libavformat/pjsdec.c |   17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavformat/pjsdec.c b/libavformat/pjsdec.c
> > index a69a316..00866b7 100644
> > --- a/libavformat/pjsdec.c
> > +++ b/libavformat/pjsdec.c
> > @@ -65,6 +65,7 @@ static int pjs_read_header(AVFormatContext *s)
> >      PJSContext *pjs = s->priv_data;
> >      AVStream *st = avformat_new_stream(s, NULL);
> >      int res = 0;
> > +    int idx;
> >  
> >      if (!st)
> >          return AVERROR(ENOMEM);
> > @@ -83,13 +84,25 @@ static int pjs_read_header(AVFormatContext *s)
> >          if (!len)
> >              break;
> >  
> > -        line[strcspn(line, "\r\n")] = 0;
> > +        idx = strcspn(line, "\r\n");
> > +        if (!line[idx]) {
> > +            av_log(s, AV_LOG_ERROR, "missing newline\n");
> > +            return AVERROR_INVALIDDATA;
> > +        }
> > +
> > +        line[idx] = 0;
> >  
> >          pts_start = read_ts(&p, &duration);
> >          if (pts_start != AV_NOPTS_VALUE) {
> >              AVPacket *sub;
> >  
> > -            p[strcspn(p, "\"")] = 0;
> > +            idx = strcspn(p, "\"");
> > +            if (!p[idx]) {
> > +                av_log(s, AV_LOG_ERROR, "missing \"\n");
> > +                return AVERROR_INVALIDDATA;
> > +            }
> > +
> > +            p[idx] = 0;
> >              sub = ff_subtitles_queue_insert(&pjs->q, p, strlen(p), 0);
> >              if (!sub)
> >                  return AVERROR(ENOMEM);
> 
> The use of strcspn() as such is fine (and we use it everywhere). I'd suggest

it is but i think the code would benefit from some checks and
warnings or errors over just keeping the pointer within the array
and producing "some" output for any arbitrary random input.


> instead:
> 
> diff --git a/libavformat/pjsdec.c b/libavformat/pjsdec.c
> index a69a316..6f5db37 100644
> --- a/libavformat/pjsdec.c
> +++ b/libavformat/pjsdec.c
> @@ -53,7 +53,8 @@ static int64_t read_ts(char **line, int *duration)
>      int64_t start, end;
>  
>      if (sscanf(*line, "%"SCNd64",%"SCNd64, &start, &end) == 2) {
> -        *line += strcspn(*line, "\"") + 1;
> +        *line += strcspn(*line, "\"");
> +        *line += !!**line;
>          *duration = end - start;
>          return start;
>      }
> 
> Which should be enough to enough to fix the problem.

applied

thanks


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140111/cd4c0cfb/attachment.asc>