Re-do abstract stream reader API.
authorTimothy B. Terriberry <tterribe@xiph.org>
Sat, 20 Oct 2012 21:12:46 +0000 (14:12 -0700)
committerTimothy B. Terriberry <tterribe@xiph.org>
Sat, 20 Oct 2012 21:37:42 +0000 (14:37 -0700)
This changes op_read_func to
a) Take a single byte count to read instead of an "item" count
    (which the http backend couldn't properly support anyway).
b) Use integers for buffer sizes to avoid having to worry about
    sign differences and whether size_t is larger or smaller than
    opus_int64, etc.
c) Return an explicit error code (instead of using errno like
    fread).
   We had already eliminated the use of errno, but we did it by
    treating read errors and EOF identically in all cases.
   This was preventing us from reporting SSL truncation attacks
    from the https backend.
   The https backend now properly reports such errors.

This commit also fixes a bug introduced in 9b57b0c2, where we
 accidentally started passing absolute offsets to the _boundary
 parameter of op_get_next_page() instead of relative offsets.
We now use absolute offsets in all places, as it is the simpler
 choice.
This matters now, because the error reported when encountering EOF
 before hitting the _boundary is no longer suppressed (but instead
 reported as OP_EBADLINK).

Finally, it removes the op_page_seek() function.
Except for the time needed to decode forward after seeking, this
 function was identical in performance to op_pcm_seek(), and Opus
 requires decoding 80 ms of data after seek anyway, so the relative
 benefit is much smaller than with Vorbis.
A survey of open-source code using libvorbisfile showed that the
 only usages of ov_page_seek() in the wild were calling it to seek
 to the start of the stream, for which op_pcm_seek() already has a
 special case that makes it just as fast.

The documentation was also updated to describe all of these chanes.

This is an incompatible API change.

examples/opusfile_example.c
examples/seeking_example.c
include/opusfile.h
src/http.c
src/opusfile.c
src/stream.c

index 1998153..0394d3b 100644 (file)
@@ -105,6 +105,7 @@ int main(int _argc,const char **_argv){
   opus_int32   bitrate;
   int          ret;
   int          prev_li;
+  int          is_ssl;
 #if defined(_WIN32)
 # undef fileno
 # define fileno _fileno
@@ -120,6 +121,7 @@ int main(int _argc,const char **_argv){
     fprintf(stderr,"Usage: %s <file.opus>\n",_argv[0]);
     return EXIT_FAILURE;
   }
+  is_ssl=0;
   if(strcmp(_argv[1],"-")==0){
     OpusFileCallbacks cb={NULL,NULL,NULL,NULL};
     of=op_open_callbacks(op_fdopen(&cb,fileno(stdin),"rb"),&cb,NULL,0,&ret);
@@ -139,6 +141,9 @@ int main(int _argc,const char **_argv){
     }
 #else
     if(of==NULL)of=op_open_file(_argv[1],&ret);
+    /*This is not a very good check, but at least it won't give false
+       positives.*/
+    else is_ssl=strncmp(_argv[1],"https:",6)==0;
 #endif
   }
   if(of==NULL){
@@ -173,6 +178,7 @@ int main(int _argc,const char **_argv){
     ret=op_read_native_stereo(of,pcm,sizeof(pcm)/sizeof(*pcm));
     if(ret<0){
       fprintf(stderr,"\nError decoding '%s': %i\n",_argv[1],ret);
+      if(is_ssl)fprintf(stderr,"Possible truncation attack?\n");
       ret=EXIT_FAILURE;
       break;
     }
index 71621d0..450911a 100644 (file)
@@ -422,29 +422,6 @@ int main(int _argc,const char **_argv){
     fprintf(stderr,"\rTotal seek operations: %li (%.3f per raw seek, %li maximum).\n",
      nreal_seeks,nreal_seeks/(double)NSEEK_TESTS,max_seeks);
     nreal_seeks=0;
-    fprintf(stderr,"Testing PCM page seeking to random places in %li "
-     "samples (",(long)pcm_length);
-    print_duration(stderr,pcm_length);
-    fprintf(stderr,")...\n");
-    max_seeks=0;
-    for(i=0;i<NSEEK_TESTS;i++){
-      long nseeks_tmp;
-      nseeks_tmp=nreal_seeks;
-      pcm_offset=(ogg_int64_t)(rand()/(double)RAND_MAX*pcm_length);
-      fprintf(stderr,"\r\t%3i [PCM position %li]...                ",
-       i,(long)pcm_offset);
-      ret=op_pcm_seek_page(of,pcm_offset);
-      if(ret<0){
-        fprintf(stderr,"\nSeek failed: %i.\n",ret);
-        nfailures++;
-      }
-      verify_seek(of,-1,pcm_offset,pcm_length,bigassbuffer);
-      nseeks_tmp=nreal_seeks-nseeks_tmp;
-      max_seeks=nseeks_tmp>max_seeks?nseeks_tmp:max_seeks;
-    }
-    fprintf(stderr,"\rTotal seek operations: %li (%.3f per page seek, %li maximum).\n",
-     nreal_seeks,nreal_seeks/(double)NSEEK_TESTS,max_seeks);
-    nreal_seeks=0;
     fprintf(stderr,"Testing exact PCM seeking to random places in %li "
      "samples (",(long)pcm_length);
     print_duration(stderr,pcm_length);
index 32f610b..e03d3d9 100644 (file)
@@ -499,19 +499,15 @@ void opus_tags_clear(OpusTags *_tags) OP_ARG_NONNULL(1);
 
 typedef struct OpusFileCallbacks OpusFileCallbacks;
 
-/**Reads \a _nmemb elements of data, each \a _size bytes long, from
-    \a _stream.
-   \return The number of items successfully read (i.e., not the number of
-            characters).
-           Unlike normal <code>fread()</code>, this function is allowed to
-            return fewer items than requested (e.g., if reading more would
-            block), as long as <em>some</em> data is returned when no error
-            occurs and EOF has not been reached.
-           If an error occurs, or the end-of-file is reached, the return
-            value is zero.
-           <code>errno</code> need not be set.*/
-typedef size_t (*op_read_func)(void *_ptr,size_t _size,size_t _nmemb,
- void *_stream);
+/**Reads up to \a _nbytes bytes of data from \a _stream.
+   \param      _stream The stream to read from.
+   \param[out] _ptr    The buffer to store the data in.
+   \param      _nbytes The maximum number of bytes to read.
+                       This function may return fewer, though it will not
+                        return zero unless it reaches end-of-file.
+   \return The number of bytes successfully read, or a negative value on
+            error.*/
+typedef int (*op_read_func)(void *_stream,unsigned char *_ptr,int _nbytes);
 
 /**Sets the position indicator for \a _stream.
    The new position, measured in bytes, is obtained by adding \a _offset
@@ -731,8 +727,6 @@ OP_WARN_UNUSED_RESULT OggOpusFile *op_open_memory(const unsigned char *_data,
  size_t _size,int *_error);
 
 /**Open a stream from a URL.
-   See the security warning in op_open_url_with_proxy() for information about
-    possible truncation attacks with HTTPS.
    This function behaves identically to op_open_url(), except that it
     takes a va_list instead of a variable number of arguments.
    It does not call the <code>va_end</code> macro, and because it invokes the
@@ -757,16 +751,6 @@ OP_WARN_UNUSED_RESULT OggOpusFile *op_vopen_url(const char *_url,
  int *_error,va_list _ap) OP_ARG_NONNULL(1);
 
 /**Open a stream from a URL.
-   \warning HTTPS streams that are not served with a Content-Length header may
-    be vulnerable to truncation attacks.
-   The abstract stream interface is incapable of signaling whether a connection
-    was closed gracefully (with a TLS "close notify" message) or abruptly (and,
-    e.g., possibly by an attacker).
-   If you wish to guarantee that you are not vulnerable to such attacks, you
-    might consider only allowing seekable streams (which must have a valid
-    content length) and verifying the file position reported by op_raw_tell()
-    after decoding to the end is at least as large as that reported by
-    op_raw_total() (though possibly larger).
    However, this approach will not work for live streams or HTTP/1.0 servers
     (which do not support Range requets).
    \param      _url   The URL to open.
@@ -1272,7 +1256,7 @@ ogg_int64_t op_pcm_tell(OggOpusFile *_of) OP_ARG_NONNULL(1);
    \param _of          The \c OggOpusFile in which to seek.
    \param _byte_offset The byte position to seek to.
    \return 0 on success, or a negative error code on failure.
-   \retval #OP_EREAD    The seek failed.
+   \retval #OP_EREAD    The underlying seek operation failed.
    \retval #OP_EINVAL   The stream was only partially open, or the target was
                          outside the valid range for the stream.
    \retval #OP_ENOSEEK  This stream is not seekable.
@@ -1280,22 +1264,6 @@ ogg_int64_t op_pcm_tell(OggOpusFile *_of) OP_ARG_NONNULL(1);
                          unknown reason.*/
 int op_raw_seek(OggOpusFile *_of,opus_int64 _byte_offset) OP_ARG_NONNULL(1);
 
-/**Seek to a page preceding the specified PCM offset, such that decoding will
-    quickly arrive at the requested position.
-   This is faster than sample-granularity seeking because it doesn't do the
-    last bit of decode to find a specific sample.
-   \param _of         The \c OggOpusFile in which to seek.
-   \param _pcm_offset The PCM offset to seek to.
-                      This is in samples at 48 kHz relative to the start of the
-                       stream.
-   \return 0 on success, or a negative value on error.
-   \retval #OP_EREAD   The seek failed.
-   \retval #OP_EINVAL  The stream was only partially open, or the target was
-                        outside the valid range for the stream.
-   \retval #OP_ENOSEEK This stream is not seekable.*/
-int op_pcm_seek_page(OggOpusFile *_of,ogg_int64_t _pcm_offset)
- OP_ARG_NONNULL(1);
-
 /**Seek to the specified PCM offset, such that decoding will begin at exactly
     the requested position.
    \param _of         The \c OggOpusFile in which to seek.
@@ -1303,10 +1271,13 @@ int op_pcm_seek_page(OggOpusFile *_of,ogg_int64_t _pcm_offset)
                       This is in samples at 48 kHz relative to the start of the
                        stream.
    \return 0 on success, or a negative value on error.
-   \retval #OP_EREAD   The seek failed.
-   \retval #OP_EINVAL  The stream was only partially open, or the target was
-                        outside the valid range for the stream.
-   \retval #OP_ENOSEEK This stream is not seekable.*/
+   \retval #OP_EREAD    An underlying read or seek operation failed.
+   \retval #OP_EINVAL   The stream was only partially open, or the target was
+                         outside the valid range for the stream.
+   \retval #OP_ENOSEEK  This stream is not seekable.
+   \retval #OP_EBADLINK We failed to find data we had seen before, or the
+                         bitstream structure was sufficiently malformed that
+                         seeking to the target destination was impossible.*/
 int op_pcm_seek(OggOpusFile *_of,ogg_int64_t _pcm_offset) OP_ARG_NONNULL(1);
 
 /*@}*/
@@ -1339,7 +1310,17 @@ int op_pcm_seek(OggOpusFile *_of,ogg_int64_t _pcm_offset) OP_ARG_NONNULL(1);
    If so, the floating-point API may also be disabled.
    In that configuration, nothing in <tt>libopusfile</tt> will use any
     floating-point operations, to simplify support on devices without an
-    adequate FPU.*/
+    adequate FPU.
+
+   \warning HTTPS streams may be be vulnerable to truncation attacks if you do
+    not check the error return code from op_read_float() or its associated
+    functions.
+   If the remote peer does not close the connection gracefully (with a TLS
+    "close notify" message), these functions will return OP_EREAD instead of 0
+    when they reach the end of the file.
+   If you are reading from an <https:> URL (particularly if seeking is not
+    supported), you should make sure to check for this error and warn the user
+    appropriately.*/
 /*@{*/
 
 /**Reads more samples from the stream.
@@ -1396,6 +1377,13 @@ int op_pcm_seek(OggOpusFile *_of,ogg_int64_t _pcm_offset) OP_ARG_NONNULL(1);
            The list of possible failure codes follows.
            Most of them can only be returned by unseekable, chained streams
             that encounter a new link.
+   \retval #OP_HOLE          There was a hole in the data, and some samples
+                              may have been skipped.
+                             Call this function again to continue decoding
+                              past the hole.
+   \retval #OP_EREAD         An underlying read operation failed.
+                             This may signal a truncation attack from an
+                              <https:> source.
    \retval #OP_EFAULT        An internal memory allocation failed.
    \retval #OP_EIMPL         An unseekable stream encountered a new link that
                               used a feature that is not implemented, such as
@@ -1411,6 +1399,7 @@ int op_pcm_seek(OggOpusFile *_of,ogg_int64_t _pcm_offset) OP_ARG_NONNULL(1);
                               an ID header that contained an unrecognized
                               version number.
    \retval #OP_EBADPACKET    Failed to properly decode the next packet.
+   \retval #OP_EBADLINK      We failed to find data we had seen before.
    \retval #OP_EBADTIMESTAMP An unseekable stream encountered a new link with
                               a starting timestamp that failed basic validity
                               checks.*/
@@ -1469,6 +1458,13 @@ OP_WARN_UNUSED_RESULT int op_read(OggOpusFile *_of,
            The list of possible failure codes follows.
            Most of them can only be returned by unseekable, chained streams
             that encounter a new link.
+   \retval #OP_HOLE          There was a hole in the data, and some samples
+                              may have been skipped.
+                             Call this function again to continue decoding
+                              past the hole.
+   \retval #OP_EREAD         An underlying read operation failed.
+                             This may signal a truncation attack from an
+                              <https:> source.
    \retval #OP_EFAULT        An internal memory allocation failed.
    \retval #OP_EIMPL         An unseekable stream encountered a new link that
                               used a feature that is not implemented, such as
@@ -1484,6 +1480,7 @@ OP_WARN_UNUSED_RESULT int op_read(OggOpusFile *_of,
                               an ID header that contained an unrecognized
                               version number.
    \retval #OP_EBADPACKET    Failed to properly decode the next packet.
+   \retval #OP_EBADLINK      We failed to find data we had seen before.
    \retval #OP_EBADTIMESTAMP An unseekable stream encountered a new link with
                               a starting timestamp that failed basic validity
                               checks.*/
@@ -1522,6 +1519,13 @@ OP_WARN_UNUSED_RESULT int op_read_float(OggOpusFile *_of,
            The list of possible failure codes follows.
            Most of them can only be returned by unseekable, chained streams
             that encounter a new link.
+   \retval #OP_HOLE          There was a hole in the data, and some samples
+                              may have been skipped.
+                             Call this function again to continue decoding
+                              past the hole.
+   \retval #OP_EREAD         An underlying read operation failed.
+                             This may signal a truncation attack from an
+                              <https:> source.
    \retval #OP_EFAULT        An internal memory allocation failed.
    \retval #OP_EIMPL         An unseekable stream encountered a new link that
                               used a feature that is not implemented, such as
@@ -1537,6 +1541,7 @@ OP_WARN_UNUSED_RESULT int op_read_float(OggOpusFile *_of,
                               an ID header that contained an unrecognized
                               version number.
    \retval #OP_EBADPACKET    Failed to properly decode the next packet.
+   \retval #OP_EBADLINK      We failed to find data we had seen before.
    \retval #OP_EBADTIMESTAMP An unseekable stream encountered a new link with
                               a starting timestamp that failed basic validity
                               checks.*/
@@ -1575,6 +1580,13 @@ OP_WARN_UNUSED_RESULT int op_read_stereo(OggOpusFile *_of,
            The list of possible failure codes follows.
            Most of them can only be returned by unseekable, chained streams
             that encounter a new link.
+   \retval #OP_HOLE          There was a hole in the data, and some samples
+                              may have been skipped.
+                             Call this function again to continue decoding
+                              past the hole.
+   \retval #OP_EREAD         An underlying read operation failed.
+                             This may signal a truncation attack from an
+                              <https:> source.
    \retval #OP_EFAULT        An internal memory allocation failed.
    \retval #OP_EIMPL         An unseekable stream encountered a new link that
                               used a feature that is not implemented, such as
@@ -1590,6 +1602,7 @@ OP_WARN_UNUSED_RESULT int op_read_stereo(OggOpusFile *_of,
                               an ID header that contained an unrecognized
                               version number.
    \retval #OP_EBADPACKET    Failed to properly decode the next packet.
+   \retval #OP_EBADLINK      We failed to find data we had seen before.
    \retval #OP_EBADTIMESTAMP An unseekable stream encountered a new link with
                               a starting timestamp that failed basic validity
                               checks.*/
index 35ed561..707fadb 100644 (file)
@@ -766,20 +766,20 @@ static void op_http_stream_clear(OpusHTTPStream *_stream){
 }
 
 static int op_http_conn_write_fully(OpusHTTPConn *_conn,
- const char *_buf,int _size){
+ const char *_buf,int _buf_size){
   struct pollfd  fd;
   SSL           *ssl_conn;
   fd.fd=_conn->fd;
   ssl_conn=_conn->ssl_conn;
-  while(_size>0){
+  while(_buf_size>0){
     int err;
     if(ssl_conn!=NULL){
       int ret;
-      ret=SSL_write(ssl_conn,_buf,_size);
+      ret=SSL_write(ssl_conn,_buf,_buf_size);
       if(ret>0){
         /*Wrote some data.*/
         _buf+=ret;
-        _size-=ret;
+        _buf_size-=ret;
         continue;
       }
       /*Connection closed.*/
@@ -793,10 +793,10 @@ static int op_http_conn_write_fully(OpusHTTPConn *_conn,
     else{
       ssize_t ret;
       errno=0;
-      ret=write(fd.fd,_buf,_size);
+      ret=write(fd.fd,_buf,_buf_size);
       if(ret>0){
         _buf+=ret;
-        _size-=ret;
+        _buf_size-=ret;
         continue;
       }
       err=errno;
@@ -859,14 +859,17 @@ static void op_http_conn_read_rate_update(OpusHTTPConn *_conn){
 
 /*Tries to read from the given connection.
   [out] _buf: Returns the data read.
-  _size:      The size of the buffer.
-  _blocking:  Whether or not to block until some data is retrieved.*/
-static ptrdiff_t op_http_conn_read(OpusHTTPConn *_conn,
- char *_buf,ptrdiff_t _size,int _blocking){
-  struct pollfd   fd;
-  SSL            *ssl_conn;
-  ptrdiff_t       nread;
-  ptrdiff_t       nread_unblocked;
+  _buf_size:  The size of the buffer.
+  _blocking:  Whether or not to block until some data is retrieved.
+  Return: A positive number of bytes read on success.
+          0:        The read would block, or the connection was closed.
+          OP_EREAD: There was a fatal read error.*/
+static int op_http_conn_read(OpusHTTPConn *_conn,
+ unsigned char *_buf,int _buf_size,int _blocking){
+  struct pollfd  fd;
+  SSL           *ssl_conn;
+  int            nread;
+  int            nread_unblocked;
   fd.fd=_conn->fd;
   ssl_conn=_conn->ssl_conn;
   nread=nread_unblocked=0;
@@ -874,7 +877,8 @@ static ptrdiff_t op_http_conn_read(OpusHTTPConn *_conn,
     int err;
     if(ssl_conn!=NULL){
       int ret;
-      ret=SSL_read(ssl_conn,_buf+nread,_size-nread);
+      ret=SSL_read(ssl_conn,_buf+nread,_buf_size-nread);
+      OP_ASSERT(ret<=_buf_size-nread);
       if(ret>0){
         /*Read some data.
           Keep going to see if there's more.*/
@@ -882,20 +886,27 @@ static ptrdiff_t op_http_conn_read(OpusHTTPConn *_conn,
         nread_unblocked+=ret;
         continue;
       }
-      /*Connection closed.*/
-      else if(ret==0)break;
       /*If we already read some data, return it right now.*/
       if(nread>0)break;
       err=SSL_get_error(ssl_conn,ret);
+      if(ret==0){
+        /*Connection close.
+          Check for a clean shutdown to prevent truncation attacks.
+          This check always succeeds for SSLv2, as it has no "close notify"
+           message and thus can't verify an orderly shutdown.*/
+        return err==SSL_ERROR_ZERO_RETURN?0:OP_EREAD;
+      }
       if(err==SSL_ERROR_WANT_READ)fd.events=POLLIN;
       /*Yes, renegotiations can cause SSL_read() to block for writing.*/
       else if(err==SSL_ERROR_WANT_WRITE)fd.events=POLLOUT;
-      else return 0;
+      /*Some other error.*/
+      else return OP_EREAD;
     }
     else{
       ssize_t ret;
       errno=0;
-      ret=read(fd.fd,_buf+nread,_size-nread);
+      ret=read(fd.fd,_buf+nread,_buf_size-nread);
+      OP_ASSERT(ret<=_buf_size-nread);
       if(ret>0){
         /*Read some data.
           Keep going to see if there's more.*/
@@ -907,7 +918,7 @@ static ptrdiff_t op_http_conn_read(OpusHTTPConn *_conn,
          right now.*/
       if(ret==0||nread>0)break;
       err=errno;
-      if(err!=EAGAIN&&err!=EWOULDBLOCK)return 0;
+      if(err!=EAGAIN&&err!=EWOULDBLOCK)return OP_EREAD;
       fd.events=POLLIN;
     }
     _conn->read_bytes+=nread_unblocked;
@@ -915,18 +926,18 @@ static ptrdiff_t op_http_conn_read(OpusHTTPConn *_conn,
     nread_unblocked=0;
     if(!_blocking)break;
     /*Need to wait to get any data at all.*/
-    if(poll(&fd,1,OP_POLL_TIMEOUT_MS)<=0)return 0;
+    if(poll(&fd,1,OP_POLL_TIMEOUT_MS)<=0)return OP_EREAD;
   }
-  while(nread<_size);
+  while(nread<_buf_size);
   _conn->read_bytes+=nread_unblocked;
   return nread;
 }
 
 /*Tries to look at the pending data for a connection without consuming it.
   [out] _buf: Returns the data at which we're peeking.
-  _size:      The size of the buffer.*/
+  _buf_size:  The size of the buffer.*/
 static int op_http_conn_peek(OpusHTTPConn *_conn,
- char *_buf,int _size){
+ char *_buf,int _buf_size){
   struct pollfd   fd;
   SSL            *ssl_conn;
   int             ret;
@@ -935,7 +946,7 @@ static int op_http_conn_peek(OpusHTTPConn *_conn,
   for(;;){
     int err;
     if(ssl_conn!=NULL){
-      ret=SSL_peek(ssl_conn,_buf,_size);
+      ret=SSL_peek(ssl_conn,_buf,_buf_size);
       /*Either saw some data or the connection was closed.*/
       if(ret>=0)return ret;
       err=SSL_get_error(ssl_conn,ret);
@@ -946,7 +957,7 @@ static int op_http_conn_peek(OpusHTTPConn *_conn,
     }
     else{
       errno=0;
-      ret=(int)recv(fd.fd,_buf,_size,MSG_PEEK);
+      ret=(int)recv(fd.fd,_buf,_buf_size,MSG_PEEK);
       /*Either saw some data or the connection was closed.*/
       if(ret>=0)return ret;
       err=errno;
@@ -1020,7 +1031,7 @@ static int op_http_conn_read_response(OpusHTTPConn *_conn,
     OP_ASSERT(size<=read_limit);
     OP_ASSERT(read_limit<=size+ret);
     /*Actually consume that data.*/
-    ret=op_http_conn_read(_conn,buf+size,read_limit-size,1);
+    ret=op_http_conn_read(_conn,(unsigned char *)buf+size,read_limit-size,1);
     if(OP_UNLIKELY(ret<=0))return OP_FALSE;
     size+=ret;
     buf[size]='\0';
@@ -2267,23 +2278,25 @@ static int op_http_conn_open_pos(OpusHTTPStream *_stream,
   If we've reached the end of this response body, parse the next response and
    keep going.
   [out] _buf: Returns the data read.
-  _size:      The size of the buffer.
-  _blocking:  Whether or not to block until some data is retrieved.*/
-static ptrdiff_t op_http_conn_read_body(OpusHTTPStream *_stream,
- OpusHTTPConn *_conn,char *_buf,ptrdiff_t _size,int _blocking){
+  _buf_size:  The size of the buffer.
+  Return: A positive number of bytes read on success.
+          0:        The connection was closed.
+          OP_EREAD: There was a fatal read error.*/
+static int op_http_conn_read_body(OpusHTTPStream *_stream,
+ OpusHTTPConn *_conn,unsigned char *_buf,int _buf_size){
   opus_int64 pos;
   opus_int64 end_pos;
   opus_int64 next_pos;
   opus_int64 content_length;
-  ptrdiff_t  nread;
+  int        nread;
   int        pipeline;
   int        ret;
   /*Currently this function can only be called on the LRU head.
     Otherwise, we'd need a _pnext pointer if we needed to close the connection,
      and re-opening it would re-organize the lists.*/
   OP_ASSERT(_stream->lru_head==_conn);
-  /*If we try an empty read, we won't be able to tell if we hit an error.*/
-  OP_ASSERT(_size>0);
+  /*We should have filterd out empty reads by this point.*/
+  OP_ASSERT(_buf_size>0);
   pos=_conn->pos;
   end_pos=_conn->end_pos;
   next_pos=_conn->next_pos;
@@ -2297,7 +2310,7 @@ static ptrdiff_t op_http_conn_read_body(OpusHTTPStream *_stream,
         Also return early if a non-blocking read was requested (regardless of
          whether we might be able to parse the next response without
          blocking).*/
-      if(content_length<=end_pos||!_blocking)return 0;
+      if(content_length<=end_pos)return 0;
       /*Otherwise, start on the next response.*/
       if(next_pos<0){
         /*We haven't issued another request yet.*/
@@ -2315,12 +2328,12 @@ static ptrdiff_t op_http_conn_read_body(OpusHTTPStream *_stream,
           /*If we're not pipelining, we should be requesting the rest.*/
           OP_ASSERT(pipeline||_conn->chunk_size==-1);
           ret=op_http_conn_open_pos(_stream,_conn,end_pos,_conn->chunk_size);
-          if(OP_UNLIKELY(ret<0))return 0;
+          if(OP_UNLIKELY(ret<0))return OP_EREAD;
         }
         else{
           /*Issue the request now (better late than never).*/
           ret=op_http_conn_send_request(_stream,_conn,pos,_conn->chunk_size,0);
-          if(OP_UNLIKELY(ret<0))return 0;
+          if(OP_UNLIKELY(ret<0))return OP_EREAD;
           next_pos=_conn->next_pos;
           OP_ASSERT(next_pos>=0);
         }
@@ -2330,7 +2343,7 @@ static ptrdiff_t op_http_conn_read_body(OpusHTTPStream *_stream,
            seeking somewhere else.*/
         OP_ASSERT(next_pos==end_pos);
         ret=op_http_conn_handle_response(_stream,_conn);
-        if(OP_UNLIKELY(ret<0))return 0;
+        if(OP_UNLIKELY(ret<0))return OP_EREAD;
         if(OP_UNLIKELY(ret>0)&&pipeline){
           opus_int64 next_end;
           next_end=_conn->next_end;
@@ -2343,18 +2356,19 @@ static ptrdiff_t op_http_conn_read_body(OpusHTTPStream *_stream,
            ||next_end-next_pos>=0&&next_end-next_pos<=0x7FFFFFFF);
           ret=op_http_conn_open_pos(_stream,_conn,next_pos,
            next_end<0?-1:(opus_int32)(next_end-next_pos));
-          if(OP_UNLIKELY(ret<0))return 0;
+          if(OP_UNLIKELY(ret<0))return OP_EREAD;
         }
-        else if(OP_UNLIKELY(ret!=0))return OP_FALSE;
+        else if(OP_UNLIKELY(ret!=0))return OP_EREAD;
       }
       pos=_conn->pos;
       end_pos=_conn->end_pos;
       content_length=_stream->content_length;
     }
     OP_ASSERT(end_pos>pos);
-    _size=OP_MIN(_size,end_pos-pos);
+    _buf_size=OP_MIN(_buf_size,end_pos-pos);
   }
-  nread=op_http_conn_read(_conn,_buf,_size,_blocking);
+  nread=op_http_conn_read(_conn,_buf,_buf_size,1);
+  if(OP_UNLIKELY(nread<0))return nread;
   pos+=nread;
   _conn->pos=pos;
   OP_ASSERT(end_pos<0||content_length>=0);
@@ -2378,24 +2392,22 @@ static ptrdiff_t op_http_conn_read_body(OpusHTTPStream *_stream,
     if(chunk_size>=0)request_thresh=OP_MIN(chunk_size>>2,request_thresh);
     if(end_pos-pos<=request_thresh){
       ret=op_http_conn_send_request(_stream,_conn,end_pos,_conn->chunk_size,1);
-      if(OP_UNLIKELY(ret<0))return 0;
+      if(OP_UNLIKELY(ret<0))return OP_EREAD;
     }
   }
   return nread;
 }
 
-static size_t op_http_stream_read(void *_ptr,size_t _size,size_t _nmemb,
void *_stream){
+static int op_http_stream_read(void *_stream,
unsigned char *_ptr,int _buf_size){
   OpusHTTPStream *stream;
   ptrdiff_t       nread;
-  ptrdiff_t       total;
   opus_int64      size;
   opus_int64      pos;
   int             ci;
   stream=(OpusHTTPStream *)_stream;
-  total=_size*_nmemb;
-  /*Check for overflow/empty read.*/
-  if(total==0||total/_size!=_nmemb||total>OP_INT64_MAX)return 0;
+  /*Check for an empty read.*/
+  if(_buf_size<=0)return 0;
   ci=stream->cur_conni;
   /*No current connection => EOF.*/
   if(ci<0)return 0;
@@ -2405,42 +2417,11 @@ static size_t op_http_stream_read(void *_ptr,size_t _size,size_t _nmemb,
   if(size>=0){
     if(pos>=size)return 0;
     /*Check for a short read.*/
-    if(total>size-pos){
-      _nmemb=(size-pos)/_size;
-      total=_size*_nmemb;
-    }
-  }
-  if(_size==1){
-    nread=op_http_conn_read_body(stream,stream->conns+ci,_ptr,total,1);
-  }
-  else{
-    ptrdiff_t n;
-    nread=0;
-    /*libopusfile doesn't read multi-byte items, but our abstract stream API
-       requires it for stdio compatibility.
-      Implement it for completeness' sake by reading individual items one at a
-       time.*/
-    do{
-      ptrdiff_t nread_item;
-      nread_item=0;
-      do{
-        /*Block on the first item, or if we've gotten a partial item.*/
-        n=op_http_conn_read_body(stream,stream->conns+ci,
-         _ptr,_size-nread_item,nread==0||nread_item>0);
-        nread_item+=n;
-      }
-      while(n>0&&nread_item<(ptrdiff_t)_size);
-      /*We can still fail to read a whole item if we encounter an error, or if
-         we hit EOF and didn't know the stream length.
-        TODO: The former is okay, the latter is not, but I don't know how to
-         fix it without buffering arbitrarily large amounts of data.*/
-      if(nread_item>=(ptrdiff_t)_size)nread++;
-      total-=_size;
-    }
-    while(n>0&&total>0);
+    if(_buf_size>size-pos)_buf_size=(int)(size-pos);
   }
+  nread=op_http_conn_read_body(stream,stream->conns+ci,_ptr,_buf_size);
   if(OP_UNLIKELY(nread<=0)){
-    /*We either hit an error or EOF.
+    /*We hit an error or EOF.
       Either way, we're done with this connection.*/
     op_http_conn_close(stream,stream->conns+ci,&stream->lru_head,1);
     stream->cur_conni=-1;
@@ -2458,7 +2439,7 @@ static size_t op_http_stream_read(void *_ptr,size_t _size,size_t _nmemb,
   _target:          The stream position to which to read ahead.*/
 static int op_http_conn_read_ahead(OpusHTTPStream *_stream,
  OpusHTTPConn *_conn,int _just_read_ahead,opus_int64 _target){
-  static char dummy_buf[OP_READAHEAD_CHUNK_SIZE];
+  static unsigned char dummy_buf[OP_READAHEAD_CHUNK_SIZE];
   opus_int64 pos;
   opus_int64 end_pos;
   opus_int64 next_pos;
@@ -2489,7 +2470,7 @@ static int op_http_conn_read_ahead(OpusHTTPStream *_stream,
       Finish off the current chunk.*/
     while(pos<end_pos){
       nread=op_http_conn_read(_conn,dummy_buf,
-       OP_MIN(end_pos-pos,OP_READAHEAD_CHUNK_SIZE),1);
+       (int)OP_MIN(end_pos-pos,OP_READAHEAD_CHUNK_SIZE),1);
       /*We failed to read ahead.*/
       if(nread<=0)return OP_FALSE;
       pos+=nread;
@@ -2514,7 +2495,7 @@ static int op_http_conn_read_ahead(OpusHTTPStream *_stream,
   }
   while(pos<end_pos){
     nread=op_http_conn_read(_conn,dummy_buf,
-     OP_MIN(end_pos-pos,OP_READAHEAD_CHUNK_SIZE),1);
+     (int)OP_MIN(end_pos-pos,OP_READAHEAD_CHUNK_SIZE),1);
     /*We failed to read ahead.*/
     if(nread<=0)return OP_FALSE;
     pos+=nread;
index 43500c9..e12f294 100644 (file)
@@ -134,18 +134,17 @@ int op_test(OpusHead *_head,
 
 /*The read/seek functions track absolute position within the stream.*/
 
-/*Read a little more data from the file/pipe into the ogg_sync framer.*/
+/*Read a little more data from the file/pipe into the ogg_sync framer.
+  Return: A positive number of bytes read on success, 0 on end-of-file, or a
+           negative value on failure.*/
 static int op_get_data(OggOpusFile *_of){
-  char *buffer;
-  int   bytes;
-  buffer=ogg_sync_buffer(&_of->oy,OP_READ_SIZE);
-  bytes=(int)(*_of->callbacks.read)(buffer,
-   1,OP_READ_SIZE,_of->source);
-  if(OP_LIKELY(bytes>0)){
-    ogg_sync_wrote(&_of->oy,bytes);
-    return bytes;
-  }
-  return OP_EREAD;
+  unsigned char *buffer;
+  int            bytes;
+  buffer=(unsigned char *)ogg_sync_buffer(&_of->oy,OP_READ_SIZE);
+  bytes=(int)(*_of->callbacks.read)(_of->source,buffer,OP_READ_SIZE);
+  OP_ASSERT(bytes<=OP_READ_SIZE);
+  if(OP_LIKELY(bytes>0))ogg_sync_wrote(&_of->oy,bytes);
+  return bytes;
 }
 
 /*Save a tiny smidge of verbosity to make the code more readable.*/
@@ -171,16 +170,16 @@ static opus_int64 op_position(OggOpusFile *_of){
 /*From the head of the stream, get the next page.
   _boundary specifies if the function is allowed to fetch more data from the
    stream (and how much) or only use internally buffered data.
-  _boundary: -1) Unbounded search.
-              0) Read no additional data.
+  _boundary: -1: Unbounded search.
+              0: Read no additional data.
                  Use only cached data.
-              n) Search for the start of a new page for n bytes.
-  Return: n>=0)     Found a page at absolute offset n.
-          OP_FALSE) Hit the _boundary limit.
-          OP_EREAD) Failed to read more data.*/
+              n: Search for the start of a new page up to file position n.
+  Return: n>=0:       Found a page at absolute offset n.
+          OP_FALSE:   Hit the _boundary limit.
+          OP_EREAD:   An underlying read operation failed.
+          OP_BADLINK: We hit end-of-file before reaching _boundary.*/
 static opus_int64 op_get_next_page(OggOpusFile *_of,ogg_page *_og,
  opus_int64 _boundary){
-  if(_boundary>0)_boundary+=_of->offset;
   for(;;){
     int more;
     if(_boundary>0&&_of->offset>=_boundary)return OP_FALSE;
@@ -192,14 +191,13 @@ static opus_int64 op_get_next_page(OggOpusFile *_of,ogg_page *_og,
       /*Send more paramedics.*/
       if(!_boundary)return OP_FALSE;
       ret=op_get_data(_of);
-      if(OP_UNLIKELY(ret<0)){
-        opus_int64 read_offset;
-        /*If we read up to the boundary (or EOF in a seekable stream),
-           including buffered sync data, then treat this as EOF.
-          Otherwise treat it as a read error.*/
-        if(_boundary<0)_boundary=_of->end;
-        read_offset=op_position(_of);
-        return read_offset>=_boundary?OP_FALSE:ret;
+      if(OP_UNLIKELY(ret<0))return OP_EREAD;
+      if(OP_UNLIKELY(ret==0)){
+        /*If we encounter an EOF, return an error if we didn't at least read up
+           to the boundary (if known).
+          This test always succeeds if _boundary is -1, but that only happens
+           in unseekable streams.*/
+        return op_position(_of)>=_boundary?OP_FALSE:OP_EBADLINK;
       }
     }
     else{
@@ -311,7 +309,7 @@ static int op_get_prev_page_serial(OggOpusFile *_of,
     while(_of->offset<end){
       opus_int64   llret;
       ogg_uint32_t serialno;
-      llret=op_get_next_page(_of,&og,end-_of->offset);
+      llret=op_get_next_page(_of,&og,end);
       if(OP_UNLIKELY(llret<OP_FALSE))return (int)llret;
       else if(llret==OP_FALSE)break;
       serialno=ogg_page_serialno(&og);
@@ -391,8 +389,9 @@ static int op_fetch_headers_impl(OggOpusFile *_of,OpusHead *_head,
         _of->ready_state=OP_STREAMSET;
       }
     }
-    /*Get the next page.*/
-    llret=op_get_next_page(_of,_og,OP_CHUNK_SIZE);
+    /*Get the next page.
+      No need to clamp _boundary as all errors become OP_ENOTFORMAT.*/
+    llret=op_get_next_page(_of,_og,_of->offset+OP_CHUNK_SIZE);
     if(OP_UNLIKELY(llret<0))return OP_ENOTFORMAT;
     /*If this page also belongs to our Opus stream, submit it and break.*/
     if(_of->ready_state==OP_STREAMSET
@@ -408,7 +407,9 @@ static int op_fetch_headers_impl(OggOpusFile *_of,OpusHead *_head,
       case 0:{
         /*Loop getting pages.*/
         for(;;){
-          if(OP_UNLIKELY(op_get_next_page(_of,_og,OP_CHUNK_SIZE)<0)){
+          /*No need to clamp _boundary as all errors become OP_EBADHEADER.*/
+          if(OP_UNLIKELY(op_get_next_page(_of,_og,
+           _of->offset+OP_CHUNK_SIZE)<0)){
             return OP_EBADHEADER;
           }
           /*If this page belongs to the correct stream, go parse it.*/
@@ -453,7 +454,8 @@ static int op_fetch_headers(OggOpusFile *_of,OpusHead *_head,
   int      ret;
   if(!_og){
     ogg_int64_t llret;
-    llret=op_get_next_page(_of,&og,OP_CHUNK_SIZE);
+    /*No need to clamp _boundary as all errors become OP_ENOTFORMAT.*/
+    llret=op_get_next_page(_of,&og,_of->offset+OP_CHUNK_SIZE);
     if(OP_UNLIKELY(llret<0))return OP_ENOTFORMAT;
     _og=&og;
   }
@@ -715,9 +717,13 @@ static int op_find_initial_pcm_offset(OggOpusFile *_of,
      least once.*/
   total_duration=0;
   do{
+    opus_int64 llret;
+    llret=op_get_next_page(_of,_og,_of->end);
     /*We should get a page unless the file is truncated or mangled.
       Otherwise there are no audio data packets in the whole logical stream.*/
-    if(OP_UNLIKELY(op_get_next_page(_of,_og,_of->end)<0)){
+    if(OP_UNLIKELY(llret<0)){
+      /*Fail if there was a read error.*/
+      if(llret<OP_FALSE)return (int)llret;
       /*Fail if the pre-skip is non-zero, since it's asking us to skip more
          samples than exist.*/
       if(_link->head.pre_skip>0)return OP_EBADTIMESTAMP;
@@ -1070,7 +1076,7 @@ static int op_bisect_forward_serialno(OggOpusFile *_of,
       if(OP_UNLIKELY(ret<0))return ret;
       last=op_get_next_page(_of,&og,_of->end);
       /*At the worst we should have hit the page at _sr[sri-1].offset.*/
-      if(OP_UNLIKELY(last<0))return OP_EBADLINK;
+      if(OP_UNLIKELY(last<0))return last<OP_FALSE?(int)last:OP_EBADLINK;
       OP_ASSERT(nsr<_csr);
       _sr[nsr].serialno=ogg_page_serialno(&og);
       _sr[nsr].gp=ogg_page_granulepos(&og);
@@ -1703,7 +1709,7 @@ static int op_fetch_and_process_page(OggOpusFile *_of,
       /*Keep reading until we get a page with the correct serialno.*/
       else _page_pos=op_get_next_page(_of,&og,_of->end);
       /*EOF: Leave uninitialized.*/
-      if(_page_pos<0)return OP_EOF;
+      if(_page_pos<0)return _page_pos<OP_FALSE?(int)_page_pos:OP_EOF;
       if(OP_LIKELY(_of->ready_state>=OP_STREAMSET)){
         if(cur_serialno!=(ogg_uint32_t)ogg_page_serialno(&og)){
           /*Two possibilities:
@@ -2007,7 +2013,7 @@ static ogg_int64_t op_get_granulepos(const OggOpusFile *_of,
   There is a danger here: missing pages or incorrect frame number information
    in the bitstream could make our task impossible.
   Account for that (it would be an error condition).*/
-static int op_pcm_seek_page_impl(OggOpusFile *_of,
+static int op_pcm_seek_page(OggOpusFile *_of,
  ogg_int64_t _target_gp,int _li){
   OggOpusLink  *link;
   ogg_page      og;
@@ -2079,9 +2085,9 @@ static int op_pcm_seek_page_impl(OggOpusFile *_of,
     }
     chunk_size=OP_CHUNK_SIZE;
     while(begin<end){
-      page_offset=op_get_next_page(_of,&og,end-_of->offset);
-      if(page_offset==OP_EREAD)return OP_EBADLINK;
+      page_offset=op_get_next_page(_of,&og,end);
       if(page_offset<0){
+        if(page_offset<OP_FALSE)return (int)page_offset;
         /*There are no more pages in our interval from our stream with a valid
            timestamp that start at position bisect or later.*/
         /*If we scanned the whole interval, we're done.*/
@@ -2179,17 +2185,6 @@ static int op_pcm_seek_page_impl(OggOpusFile *_of,
   return 0;
 }
 
-int op_pcm_seek_page(OggOpusFile *_of,ogg_int64_t _pcm_offset){
-  ogg_int64_t target_gp;
-  int         li;
-  if(OP_UNLIKELY(_of->ready_state<OP_OPENED))return OP_EINVAL;
-  if(OP_UNLIKELY(!_of->seekable))return OP_ENOSEEK;
-  if(OP_UNLIKELY(_pcm_offset<0))return OP_EINVAL;
-  target_gp=op_get_granulepos(_of,_pcm_offset,&li);
-  if(OP_UNLIKELY(target_gp==-1))return OP_EINVAL;
-  return op_pcm_seek_page_impl(_of,target_gp,li);
-}
-
 int op_pcm_seek(OggOpusFile *_of,ogg_int64_t _pcm_offset){
   OggOpusLink *link;
   ogg_int64_t  pcm_start;
@@ -2206,7 +2201,7 @@ int op_pcm_seek(OggOpusFile *_of,ogg_int64_t _pcm_offset){
   if(OP_UNLIKELY(_pcm_offset<0))return OP_EINVAL;
   target_gp=op_get_granulepos(_of,_pcm_offset,&li);
   if(OP_UNLIKELY(target_gp==-1))return OP_EINVAL;
-  ret=op_pcm_seek_page_impl(_of,target_gp,li);
+  ret=op_pcm_seek_page(_of,target_gp,li);
   /*Now skip samples until we actually get to our target.*/
   link=_of->links+li;
   pcm_start=link->pcm_start;
index e88e77a..25a96f9 100644 (file)
@@ -41,6 +41,18 @@ struct OpusMemStream{
   ptrdiff_t            pos;
 };
 
+static int op_fread(void *_stream,unsigned char *_ptr,int _buf_size){
+  FILE   *stream;
+  size_t  ret;
+  /*Check for empty read.*/
+  if(_buf_size<=0)return 0;
+  stream=(FILE *)_stream;
+  ret=fread(_ptr,1,_buf_size,stream);
+  OP_ASSERT(ret<=(size_t)_buf_size);
+  /*If ret==0 and !feof(stream), there was a read error.*/
+  return ret>0||feof(stream)?(int)ret:OP_EREAD;
+}
+
 static int op_fseek(void *_stream,opus_int64 _offset,int _whence){
 #if defined(_MSC_VER)
   return _fseeki64((FILE *)_stream,_offset,_whence);
@@ -58,7 +70,7 @@ static opus_int64 op_ftell(void *_stream){
 }
 
 static const OpusFileCallbacks OP_FILE_CALLBACKS={
-  (op_read_func)fread,
+  op_fread,
   op_fseek,
   op_ftell,
   (op_close_func)fclose
@@ -86,29 +98,23 @@ void *op_freopen(OpusFileCallbacks *_cb,const char *_path,const char *_mode,
   return fp;
 }
 
-static size_t op_mem_read(void *_ptr,size_t _size,size_t _nmemb,void *_stream){
+static int op_mem_read(void *_stream,unsigned char *_ptr,int _buf_size){
   OpusMemStream *stream;
-  size_t         total;
   ptrdiff_t      size;
   ptrdiff_t      pos;
   stream=(OpusMemStream *)_stream;
-  if(_size>OP_MEM_SIZE_MAX)return 0;
-  total=_size*_nmemb;
-  /*Check for overflow/empty read.*/
-  if(total==0||total/_size!=_nmemb)return 0;
+  /*Check for empty read.*/
+  if(_buf_size<=0)return 0;
   size=stream->size;
   pos=stream->pos;
   /*Check for EOF.*/
   if(pos>=size)return 0;
   /*Check for a short read.*/
-  if(total>(size_t)(size-pos)){
-    _nmemb=(size-pos)/_size;
-    total=_size*_nmemb;
-  }
-  memcpy(_ptr,stream->data+pos,total);
-  pos+=total;
+  _buf_size=(int)OP_MAX(size-pos,_buf_size);
+  memcpy(_ptr,stream->data+pos,_buf_size);
+  pos+=_buf_size;
   stream->pos=pos;
-  return _nmemb;
+  return _buf_size;
 }
 
 static int op_mem_seek(void *_stream,opus_int64 _offset,int _whence){