Sanity improvements to oc_dec_headerin() error returns.
authorTimothy B. Terriberry <tterribe@xiph.org>
Wed, 27 Jul 2016 01:29:39 +0000 (18:29 -0700)
committerTimothy B. Terriberry <tterribe@xiph.org>
Wed, 27 Jul 2016 01:44:39 +0000 (18:44 -0700)
In the case where we got a data packet before receiving all three
 headers, the old code would check to see if the second through
 seventh bytes matched the magic string "theora" (extremely
 unlikely, but possible), and if so, return TH_EBADHEADER, otherwise
 return TH_ENOTFORMAT.
That this was not consistent was a bit non-sensical.

5a5f5bb20c74 changed to returning TH_EBADHEADER if we got a data
 packet after receiving the first header, but left the old behavior
 for the first packet.
Change instead to explicitly return TH_ENOTFORMAT if the first
 header was missing (since we only check one bit of the packet to
 determine whether or not it's a data packet, odds are it's a packet
 for some other kind of data, like a Vorbis header).
We continue to return TH_EBADHEADER if we see a data packet after
 encountering a valid header, but before reading all three.

Also re-arrange the NULL checks to follow continue to allow the
 undocumented ability to pass in NULL for parameters which are not
 needed by the next header in the sequence.
E.g., it's perfectly all right to pass NULL for _setup when
 expecting to read the comment header next.
In this case we'll now return TH_EBADHEADER instead of TH_EFAULT if
 the packet was actually a data packet.

lib/decinfo.c

index 4e21e64..80e3f03 100644 (file)
@@ -172,16 +172,23 @@ static int oc_dec_headerin(oc_pack_buf *_opb,th_info *_info,
   int  ret;
   val=oc_pack_read(_opb,8);
   packtype=(int)val;
-  /*If we're at a data packet and we have received all three headers, we're
-     done.*/
-  if(!(packtype&0x80)&&_info->frame_width>0){
-    /*Our documentation says we'll return TH_EFAULT if these are NULL, so let's
-       check.*/
-    if(_tc==NULL||_setup==NULL)return TH_EFAULT;
-    /*Otherwise, if we didn't get all of the headers, this was not the next
-       packet in the expected sequence.*/
-    else if(_tc->vendor==NULL||*_setup==NULL)return TH_EBADHEADER;
-    /*Otherwise, we're done.*/
+  /*If we're at a data packet...*/
+  if(!(packtype&0x80)){
+    /*Check to make sure we received all three headers...
+      If we haven't seen any valid headers, assume this is not actually
+       Theora.*/
+    if(_info->frame_width<=0)return TH_ENOTFORMAT;
+    /*Follow our documentation, which says we'll return TH_EFAULT if this
+       are NULL (_info was checked by our caller).*/
+    if(_tc==NULL)return TH_EFAULT;
+    /*And if any other headers were missing, declare this packet "out of
+       sequence" instead.*/
+    if(_tc->vendor==NULL)return TH_EBADHEADER;
+    /*Don't check this until it's needed, since we allow passing NULL for the
+       arguments that we're not expecting the next header to fill in yet.*/
+    if(_setup==NULL)return TH_EFAULT;
+    if(*_setup==NULL)return TH_EBADHEADER;
+    /*If we got everything, we're done.*/
     return 0;
   }
   /*Check the codec string.*/