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

[Rémi Denis-Courmont]

Le jeudi 30 août 2012 06:18:44 Sergey Radionov, vous avez écrit :
> ---
>  modules/demux/mjpeg.c |   94
> ++++++++++++++++++++++++++++++++++++++++++++++--- 1 
files changed, 89
> insertions(+), 5 deletions(-)
> 
> diff --git a/modules/demux/mjpeg.c b/modules/demux/mjpeg.c
> index a057db3..f5bd1dd 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 + strlen("boundary=") ); 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,92 @@ 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;
> +            char *content_type = stream_ContentType( p_demux->s );
> +            char* boundary = NULL;
> +            if ( content_type != NULL )
> +            {
> +                boundary = strstr( content_type, "boundary=" );
> +                if( boundary ) boundary += strlen("boundary=");
> +            }

That's not a correct way to parse the content type per specification.

> +
> +            //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)... +
> +            size_t boundary_len = boundary ? strlen(boundary) : 0;
> +            //if used boundary without additional "--"
> +            //(i.e. used same string was declared in ContentType), then
> +            if( boundary && !strncmp( psz_line, boundary, boundary_len ) )
> +            {
> +                //if boundary identical to psz_line, then
> +                if( *(psz_line + boundary_len) == '\0' )
> +                {
> +                    //just use whole line as separator
> +                    p_sys->psz_separator = psz_line;
> +                }
> +                else
> +                {
> +                    //restore buffer position to right place
> +                    i_pos = i_pos_save + boundary_len;
> +                    p_sys->psz_separator = strdup( boundary );
> +                    free( psz_line );
> +                    psz_line = NULL;
> +
> +                    if( !p_sys->psz_separator )
> +                    {
> +                        free( content_type );
> +                        *p_header_size = -4;
> +                        return false;
> +                    }
> +                }
> +            }
> +
> +            //if used boundary with additional "--"
> +            if( !p_sys->psz_separator && boundary &&
> +                !strncmp( psz_line + strlen("--"), boundary, boundary_len
> ) ) +            {
> +                char* s = psz_line + strlen("--") + boundary_len;
> +                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 + boundary_len;
> +                }
> +
> +                p_sys->psz_separator = strdup( boundary );
> +                free( psz_line );
> +                psz_line = NULL;
> +
> +                if( !p_sys->psz_separator )
> +                {
> +                    free( content_type );
> +                    *p_header_size = -4;
> +                    return false;
> +                }
> +            }
> +
> +            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;
> +            }
> +

This entire block looks completely insane. I can't understand it but it 
definitely does not look like what the specification says.

>              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 separator_len = strlen(p_sys->psz_separator);
> +            if( strncmp( psz_line + strlen("--"), p_sys->psz_separator,
> separator_len ) && +                strncmp( psz_line,
> p_sys->psz_separator, separator_len ) ) {
>                  msg_Warn( p_demux, "separator %s does not match %s",
> psz_line, p_sys->psz_separator );
> @@ -527,7 +607,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;

No way. Absolutely no way. The two leading dashes are *_MANDATORY_*.

Skipping them is not only against the specification, it will cause too many 
false positives in matching the file format.

Fix the broken software instead.

-- 
Rémi Denis-Courmont
http://www.linkedin.com/in/remidenis