[vlc-devel] [PATCH] added handling of wrong MIME boundary separator to mjpeg demux
Sergey Radionov
rsatom at gmail.com
Thu Aug 30 03:10:33 CEST 2012
2012/8/30 Hugo Beauzée-Luyssen <hugo at beauzee.fr>:
> 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
sizeof calculated in compile type, strlen in runtime, so sizeof is
faster, but OK, if you stand on it.
> This applies to other use of sizeof(<litteral string>) - 1 in your patch
> Also you should check for the return of strdup
Accepted.
>
>> 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
I have too poor english, can you point to mistakes?
>
>> + 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)
Accepted.
>
>> + //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
Shame on me! Accepted.
>
>> + {
>> + //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)
Sorry, Accepted.
> 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)
it's just insurance for possible future changes, but Ok, Accepted.
>
>> + 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.
Ok, I'll make it as soon as possible.
>
> Regards,
>
> --
> Hugo Beauzée-Luyssen
> mail: hugo at beauzee.fr
> skype: beauze.h
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list