[vlc-devel] [PATCH] added handling of wrong MIME boundary separator to mjpeg demux

[Hugo Beauzée-Luyssen]

On Sun, Aug 26, 2012 at 1:06 PM, Sergey Radionov <rsatom at gmail.com> wrote:
> ---
>  modules/demux/mjpeg.c |   83 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 76 insertions(+), 7 deletions(-)
>
> diff --git a/modules/demux/mjpeg.c b/modules/demux/mjpeg.c
> index a057db3..055a908 100644
> --- a/modules/demux/mjpeg.c
> +++ b/modules/demux/mjpeg.c
> @@ -200,10 +200,10 @@ static bool CheckMimeHeader( demux_t *p_demux, int *p_header_size )
>              char *content_type = stream_ContentType( p_demux->s );
>              if ( content_type == NULL )
>                  return false;
> -            const char* boundary = strstr( content_type, "boundary=--" );
> +            const char* boundary = strstr( content_type, "boundary=" );
>              if ( boundary != NULL )
>              {
> -                p_sys->psz_separator = strdup( boundary + strlen( "boundary=--" ) );
> +                p_sys->psz_separator = strdup( boundary + sizeof("boundary=") - 1 );
I would go for strlen("boundary=") instead of sizeof() - 1
This applies to other use of sizeof(<litteral string>) - 1 in your patch
Also you should check for the return of strdup

>                  msg_Dbg( p_demux, "Video boundary extracted from Content-Type: %s", p_sys->psz_separator );
>                  free( content_type );
>                  /* Skip to HTTP header parsing as there's no boundary to extract
> @@ -223,7 +223,8 @@ static bool CheckMimeHeader( demux_t *p_demux, int *p_header_size )
>      }
>      else
>      {
> -        i_pos = *p_sys->p_peek == '-' ? 2 : 4;
> +        i_pos = *p_sys->p_peek == '-' ? 0 : 2;//skip "\r\n" if exist
> +        int i_pos_save = i_pos;//to allow restore after missing "\r\n"...
>          psz_line = GetLine( p_demux, &i_pos );
>          if( NULL == psz_line )
>          {
> @@ -235,13 +236,77 @@ static bool CheckMimeHeader( demux_t *p_demux, int *p_header_size )
>          /* Read the separator and remember it if not yet stored */
>          if( p_sys->psz_separator == NULL )
>          {
> -            p_sys->psz_separator = psz_line;
> -            msg_Dbg( p_demux, "Multipart MIME detected, using separator: %s",
> -                     p_sys->psz_separator );
> +            char *content_type = stream_ContentType( p_demux->s );
> +            char* boundary = NULL;
> +            if ( content_type != NULL )
> +            {
> +                boundary = strstr( content_type, "boundary=" );
> +                if( boundary ) boundary += sizeof("boundary=") - 1;
> +            }
> +
> +            //some vendors add "--" at boundary begin and end in ContentType declaration,
> +            //and don't add additional "--" in the body - it's incorrect...
> +            //some other vendors, forget to add "\r\n" after boundary - it's incorrect too...
> +            //and some else vendors, forget to add "\r\n" and forget to add "--" both
> +            //(or add excess "--" to ContentType, which is the same)...
> +
Grammar might be improved

> +            size_t bl = boundary ? strlen(boundary) : 0;
Please make the variable a bit more explicit (even though it's clear
that it design the boundary_length, I think it's easier to read with a
longer name)

> +            //if used boundary without additional "--"
> +            //(i.e. used same string was declared in ContentType), then
> +            if( boundary && !strncmp( psz_line, boundary, bl ) )
> +            {
> +                //if boundary identical to psz_line, then
> +                if( (psz_line + bl) == '\0' )
I'm surprised this doesn't warn, but I'm pretty sure you're actually
testing for the value of the pointer against 0, which is probably not
what you want.
You have that kind of error afterward in this patch, same remark applies

> +                {
> +                    //just use whole line as separator
> +                    p_sys->psz_separator = psz_line;
> +                }
> +                else
> +                {
> +                    //restore buffer position to right place
> +                    i_pos = i_pos_save + bl;
> +                    p_sys->psz_separator = strdup( boundary );
> +                    free( psz_line );
> +                }
> +            }
> +
> +            //if used boundary with additional "--"
> +            if( !p_sys->psz_separator && boundary &&
> +                !strncmp( psz_line + sizeof("--") - 1, boundary, bl ) )
> +            {
> +                char* s = psz_line + sizeof("--") - 1 + bl;
> +                if( !strncmp( s , "--", 2 ) ) //skip "--" if boundary is last
> +                    s += 2;
> +
> +                //"\r\n" is forgotten
> +                if ( s != '\0' ) {
> +                    //restore buffer position to right place
> +                    i_pos = i_pos_save + bl;
> +                }
> +
> +                p_sys->psz_separator = strdup( boundary );
> +                free( psz_line );
> +            }
> +
> +            if( !p_sys->psz_separator )
> +            {
> +                //separator was not found, then just use whole line
> +                //FIXME: don't sure it's right
> +                p_sys->psz_separator = psz_line;
> +            }
> +
> +            if ( p_sys->psz_separator ) {
Please keep the braces style consistant (ie. on a new line)
Also, if i'm not mistaking, psz_separator is guaranteed to be non NULL
here, so the check isn't needed (actually I'm pretty sure msg_*
handles NULL strings)

> +                msg_Dbg( p_demux, "Multipart MIME detected, using separator: %s",
> +                         p_sys->psz_separator );
> +            }
> +
> +            free( content_type );
>          }
>          else
>          {
> -            if( strcmp( psz_line, p_sys->psz_separator ) )
> +            size_t sl = strlen(p_sys->psz_separator);
> +            if( strncmp( psz_line + sizeof("--") - 1, p_sys->psz_separator, sl ) &&
> +                strncmp( psz_line, p_sys->psz_separator, sl ) )
>              {
>                  msg_Warn( p_demux, "separator %s does not match %s", psz_line,
>                            p_sys->psz_separator );
> @@ -527,7 +592,11 @@ static int MimeDemux( demux_t *p_demux )
>              }
>          }
>
> +        //some vendors use as separator same string which was defined in ContentType
> +        //without additional "--"
>          if( !strncmp( p_sys->psz_separator, (char *)(p_sys->p_peek + i + 2),
> +                      strlen( p_sys->psz_separator ) ) ||
> +            !strncmp( p_sys->psz_separator, (char *)(p_sys->p_peek + i),
>                        strlen( p_sys->psz_separator ) ) )
>          {
>              break;
> --
> 1.7.7.1.msysgit.0
>

To be fair I didn't yet check the logic of your patch against some
mjpeg streams that worked before, but some part of the logic is broken
(char* == '\0') for instance. I'll review again once this point is
fixed.

Regards,

-- 
Hugo Beauzée-Luyssen
mail: hugo at beauzee.fr
skype: beauze.h