* modules/codec/spudec: fixed various (innocuous) buffer overflows and

    paved the way for private SPU commands.

modules/codec/spudec/parse.c

 /*****************************************************************************
  * parse.c: SPU parser
  *****************************************************************************
- * Copyright (C) 2000-2001, 2005 the VideoLAN team
+ * Copyright (C) 2000-2001, 2005, 2006 the VideoLAN team
  * $Id$
  *
- * Authors: Samuel Hocevar <sam@zoy.org>
+ * Authors: Sam Hocevar <sam@zoy.org>
  *          Laurent Aimar <fenrir@via.ecp.fr>
  *          Gildas Bazin <gbazin@videolan.org>
  *
@@ -149,21 +149,18 @@ static int ParseControlSeq( decoder_t *p_dec, subpicture_t *p_spu,
     p_spu->i_start = p_spu->i_stop = 0;
     p_spu->b_ephemer = VLC_FALSE;
 
-    do
+    for( i_index = 4 + p_sys->i_rle_size; i_index < p_sys->i_spu_size ; )
     {
-        if( (int)i_index >= p_sys->i_spu_size + 1 )
-        {
-            /* sanity
-             * XXX only on test by loop as p_sys->buffer is bigger than needed
-             * to avoid checking at each access
-             */
-            break;
-        }
-
         /* If we just read a command sequence, read the next one;
          * otherwise, go on with the commands of the current sequence. */
         if( i_command == SPU_CMD_END )
         {
+            if( i_index + 4 > p_sys->i_spu_size )
+            {
+                msg_Err( p_dec, "overflow in SPU command sequence" );
+                return VLC_EGENERIC;
+            }
+
             /* Get the control sequence date */
             date = (mtime_t)GetWBE( &p_sys->buffer[i_index] ) * 11000;
             /* FIXME How to access i_rate
@@ -174,41 +171,56 @@ static int ParseControlSeq( decoder_t *p_dec, subpicture_t *p_spu,
             i_cur_seq = i_index;
             i_next_seq = GetWBE( &p_sys->buffer[i_index+2] );
 
+            if( i_next_seq > p_sys->i_spu_size )
+            {
+                msg_Err( p_dec, "overflow in SPU next command sequence" );
+                return VLC_EGENERIC;
+            }
+
             /* Skip what we just read */
             i_index += 4;
         }
 
-        i_command = p_sys->buffer[i_index++];
+        i_command = p_sys->buffer[i_index];
 
         switch( i_command )
         {
         case SPU_CMD_FORCE_DISPLAY: /* 00 (force displaying) */
             p_spu->i_start = p_spu_data->i_pts + date;
             p_spu->b_ephemer = VLC_TRUE;
+            i_index += 1;
             break;
 
         /* Convert the dates in seconds to PTS values */
         case SPU_CMD_START_DISPLAY: /* 01 (start displaying) */
             p_spu->i_start = p_spu_data->i_pts + date;
+            i_index += 1;
             break;
 
         case SPU_CMD_STOP_DISPLAY: /* 02 (stop displaying) */
             p_spu->i_stop = p_spu_data->i_pts + date;
+            i_index += 1;
             break;
 
         case SPU_CMD_SET_PALETTE:
 
             /* 03xxxx (palette) */
+            if( i_index + 3 > p_sys->i_spu_size )
+            {
+                msg_Err( p_dec, "overflow in SPU command" );
+                return VLC_EGENERIC;
+            }
+
             if( p_dec->fmt_in.subs.spu.palette[0] == 0xBeeF )
             {
                 unsigned int idx[4];
 
                 p_spu_data->b_palette = VLC_TRUE;
 
-                idx[0] = (p_sys->buffer[i_index+0]>>4)&0x0f;
-                idx[1] = (p_sys->buffer[i_index+0])&0x0f;
-                idx[2] = (p_sys->buffer[i_index+1]>>4)&0x0f;
-                idx[3] = (p_sys->buffer[i_index+1])&0x0f;
+                idx[0] = (p_sys->buffer[i_index+1]>>4)&0x0f;
+                idx[1] = (p_sys->buffer[i_index+1])&0x0f;
+                idx[2] = (p_sys->buffer[i_index+2]>>4)&0x0f;
+                idx[3] = (p_sys->buffer[i_index+2])&0x0f;
 
                 for( i = 0; i < 4 ; i++ )
                 {
@@ -220,15 +232,21 @@ static int ParseControlSeq( decoder_t *p_dec, subpicture_t *p_spu,
                     p_spu_data->pi_yuv[3-i][2] = (i_color>>8) & 0xff;
                 }
             }
-            i_index += 2;
 
+            i_index += 3;
             break;
 
         case SPU_CMD_SET_ALPHACHANNEL: /* 04xxxx (alpha channel) */
-            pi_alpha[3] = (p_sys->buffer[i_index+0]>>4)&0x0f;
-            pi_alpha[2] = (p_sys->buffer[i_index+0])&0x0f;
-            pi_alpha[1] = (p_sys->buffer[i_index+1]>>4)&0x0f;
-            pi_alpha[0] = (p_sys->buffer[i_index+1])&0x0f;
+            if( i_index + 3 > p_sys->i_spu_size )
+            {
+                msg_Err( p_dec, "overflow in SPU command" );
+                return VLC_EGENERIC;
+            }
+
+            pi_alpha[3] = (p_sys->buffer[i_index+1]>>4)&0x0f;
+            pi_alpha[2] = (p_sys->buffer[i_index+1])&0x0f;
+            pi_alpha[1] = (p_sys->buffer[i_index+2]>>4)&0x0f;
+            pi_alpha[0] = (p_sys->buffer[i_index+2])&0x0f;
 
             /* Ignore blank alpha palette. Sometimes spurious blank
              * alpha palettes are present - dunno why. */
@@ -244,39 +262,73 @@ static int ParseControlSeq( decoder_t *p_dec, subpicture_t *p_spu,
                 msg_Warn( p_dec, "ignoring blank alpha palette" );
             }
 
-            i_index += 2;
+            i_index += 3;
             break;
 
         case SPU_CMD_SET_COORDINATES: /* 05xxxyyyxxxyyy (coordinates) */
-            p_spu->i_x = (p_sys->buffer[i_index+0]<<4)|
-                         ((p_sys->buffer[i_index+1]>>4)&0x0f);
-            p_spu->i_width = (((p_sys->buffer[i_index+1]&0x0f)<<8)|
-                              p_sys->buffer[i_index+2]) - p_spu->i_x + 1;
+            if( i_index + 7 > p_sys->i_spu_size )
+            {
+                msg_Err( p_dec, "overflow in SPU command" );
+                return VLC_EGENERIC;
+            }
 
-            p_spu->i_y = (p_sys->buffer[i_index+3]<<4)|
-                         ((p_sys->buffer[i_index+4]>>4)&0x0f);
-            p_spu->i_height = (((p_sys->buffer[i_index+4]&0x0f)<<8)|
-                              p_sys->buffer[i_index+5]) - p_spu->i_y + 1;
+            p_spu->i_x = (p_sys->buffer[i_index+1]<<4)|
+                         ((p_sys->buffer[i_index+2]>>4)&0x0f);
+            p_spu->i_width = (((p_sys->buffer[i_index+2]&0x0f)<<8)|
+                              p_sys->buffer[i_index+3]) - p_spu->i_x + 1;
+
+            p_spu->i_y = (p_sys->buffer[i_index+4]<<4)|
+                         ((p_sys->buffer[i_index+5]>>4)&0x0f);
+            p_spu->i_height = (((p_sys->buffer[i_index+5]&0x0f)<<8)|
+                              p_sys->buffer[i_index+6]) - p_spu->i_y + 1;
 
             /* Auto crop fullscreen subtitles */
             if( p_spu->i_height > 250 )
                 p_spu_data->b_auto_crop = VLC_TRUE;
 
-            i_index += 6;
+            i_index += 7;
             break;
 
         case SPU_CMD_SET_OFFSETS: /* 06xxxxyyyy (byte offsets) */
-            p_spu_data->pi_offset[0] = GetWBE(&p_sys->buffer[i_index+0]) - 4;
-            p_spu_data->pi_offset[1] = GetWBE(&p_sys->buffer[i_index+2]) - 4;
-            i_index += 4;
+            if( i_index + 5 > p_sys->i_spu_size )
+            {
+                msg_Err( p_dec, "overflow in SPU command" );
+                return VLC_EGENERIC;
+            }
+
+            p_spu_data->pi_offset[0] = GetWBE(&p_sys->buffer[i_index+1]) - 4;
+            p_spu_data->pi_offset[1] = GetWBE(&p_sys->buffer[i_index+3]) - 4;
+            i_index += 5;
             break;
 
         case SPU_CMD_END: /* ff (end) */
+            i_index += 1;
             break;
 
         default: /* xx (unknown command) */
-            msg_Warn( p_dec, "unknown command 0x%.2x", i_command );
-            return VLC_EGENERIC;
+            msg_Warn( p_dec, "unknown SPU command 0x%.2x", i_command );
+            if( i_index + 1 < i_next_seq )
+            {
+                 /* There is at least one other command sequence */
+                 if( p_sys->buffer[i_next_seq - 1] == SPU_CMD_END )
+                 {
+                     /* This is consistent. Skip to that command sequence. */
+                     i_index = i_next_seq;
+                 }
+                 else
+                 {
+                     /* There were other commands. */
+                     msg_Warn( p_dec, "cannot recover, dropping subtitle" );
+                     return VLC_EGENERIC;
+                 }
+            }
+            else
+            {
+                /* We were in the last command sequence. Stop parsing by
+                 * pretending we met an SPU_CMD_END command. */
+                i_command = SPU_CMD_END;
+                i_index++;
+            }
         }
 
         /* We need to check for quit commands here */
@@ -285,7 +337,11 @@ static int ParseControlSeq( decoder_t *p_dec, subpicture_t *p_spu,
             return VLC_EGENERIC;
         }
 
-    } while( i_command != SPU_CMD_END || i_index == i_next_seq );
+        if( i_command == SPU_CMD_END && i_index != i_next_seq )
+        {
+            break;
+        }
+    }
 
     /* Check that the next sequence index matches the current one */
     if( i_next_seq != i_cur_seq )
@@ -295,7 +351,7 @@ static int ParseControlSeq( decoder_t *p_dec, subpicture_t *p_spu,
         return VLC_EGENERIC;
     }
 
-    if( (int)i_index > p_sys->i_spu_size )
+    if( i_index > p_sys->i_spu_size )
     {
         msg_Err( p_dec, "uh-oh, we went too far (0x%.4x > 0x%.4x)",
                  i_index, p_sys->i_spu_size );
@@ -315,11 +371,11 @@ static int ParseControlSeq( decoder_t *p_dec, subpicture_t *p_spu,
     }
 
     /* Get rid of padding bytes */
-    if( p_sys->i_spu_size > (int)i_index + 1 )
+    if( p_sys->i_spu_size > i_index + 1 )
     {
-        /* Zero or one padding byte, are quite usual
+        /* Zero or one padding byte are quite usual
          * More than one padding byte - this is very strange, but
-         * we can deal with it */
+         * we can ignore them. */
         msg_Warn( p_dec, "%i padding bytes, we usually get 0 or 1 of them",
                   p_sys->i_spu_size - i_index );
     }

modules/codec/spudec/spudec.c

 /*****************************************************************************
  * spudec.c : SPU decoder thread
  *****************************************************************************
- * Copyright (C) 2000-2001 the VideoLAN team
+ * Copyright (C) 2000-2001, 2006 the VideoLAN team
  * $Id$
  *
- * Authors: Samuel Hocevar <sam@zoy.org>
+ * Authors: Sam Hocevar <sam@zoy.org>
  *          Laurent Aimar <fenrir@via.ecp.fr>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -119,7 +119,11 @@ static void Close( vlc_object_t *p_this )
     decoder_t     *p_dec = (decoder_t*)p_this;
     decoder_sys_t *p_sys = p_dec->p_sys;
 
-    if( p_sys->p_block ) block_ChainRelease( p_sys->p_block );
+    if( p_sys->p_block )
+    {
+        block_ChainRelease( p_sys->p_block );
+    }
+
     free( p_sys );
 }
 
@@ -130,28 +134,30 @@ static subpicture_t *Decode( decoder_t *p_dec, block_t **pp_block )
 {
     decoder_sys_t *p_sys = p_dec->p_sys;
     block_t       *p_spu_block;
+    subpicture_t  *p_spu;
 
-    if( (p_spu_block = Reassemble( p_dec, pp_block )) )
-    {
-        subpicture_t *p_spu;
+    p_spu_block = Reassemble( p_dec, pp_block );
 
-        p_sys->i_spu = block_ChainExtract( p_spu_block, p_sys->buffer, 65536 );
-        p_sys->i_pts = p_spu_block->i_pts;
-        block_ChainRelease( p_spu_block );
+    if( ! p_spu_block )
+    {
+        return NULL;
+    }
 
-        /* Parse and decode */
-        p_spu = E_(ParsePacket)( p_dec );
+    /* FIXME: what the, we shouldn’t need to allocate 64k of buffer --sam. */
+    p_sys->i_spu = block_ChainExtract( p_spu_block, p_sys->buffer, 65536 );
+    p_sys->i_pts = p_spu_block->i_pts;
+    block_ChainRelease( p_spu_block );
 
-        /* reinit context */
-        p_sys->i_spu_size = 0;
-        p_sys->i_rle_size = 0;
-        p_sys->i_spu      = 0;
-        p_sys->p_block    = NULL;
+    /* Parse and decode */
+    p_spu = E_(ParsePacket)( p_dec );
 
-        return p_spu;
-    }
+    /* reinit context */
+    p_sys->i_spu_size = 0;
+    p_sys->i_rle_size = 0;
+    p_sys->i_spu      = 0;
+    p_sys->p_block    = NULL;
 
-    return NULL;
+    return p_spu;
 }
 
 /*****************************************************************************
@@ -162,21 +168,21 @@ static block_t *Packetize( decoder_t *p_dec, block_t **pp_block )
     decoder_sys_t *p_sys = p_dec->p_sys;
     block_t       *p_spu = Reassemble( p_dec, pp_block );
 
-    if( p_spu )
+    if( ! p_spu )
     {
-        p_spu->i_dts = p_spu->i_pts;
-        p_spu->i_length = 0;
+        return NULL;
+    }
 
-        /* reinit context */
-        p_sys->i_spu_size = 0;
-        p_sys->i_rle_size = 0;
-        p_sys->i_spu      = 0;
-        p_sys->p_block    = NULL;
+    p_spu->i_dts = p_spu->i_pts;
+    p_spu->i_length = 0;
 
-        return block_ChainGather( p_spu );
-    }
+    /* reinit context */
+    p_sys->i_spu_size = 0;
+    p_sys->i_rle_size = 0;
+    p_sys->i_spu      = 0;
+    p_sys->p_block    = NULL;
 
-    return NULL;
+    return block_ChainGather( p_spu );
 }
 
 /*****************************************************************************

modules/codec/spudec/spudec.h

 /*****************************************************************************
  * spudec.h : sub picture unit decoder thread interface
  *****************************************************************************
- * Copyright (C) 1999, 2000 the VideoLAN team
+ * Copyright (C) 1999, 2000, 2006 the VideoLAN team
  * $Id$
  *
- * Authors: Samuel Hocevar <sam@zoy.org>
+ * Authors: Sam Hocevar <sam@zoy.org>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -26,14 +26,14 @@ struct decoder_sys_t
     int b_packetizer;
 
     mtime_t i_pts;
-    int i_spu_size;
-    int i_rle_size;
-    int i_spu;
+    unsigned int i_spu_size;
+    unsigned int i_rle_size;
+    unsigned int i_spu;
 
     block_t *p_block;
 
-    /* We will never overflow more than 11 bytes if I'm right */
-    uint8_t buffer[65536 + 20 ];
+    /* We will never overflow */
+    uint8_t buffer[65536];
 };
 
 typedef struct subpicture_data_t