shithub: opusfile

Download patch

ref: 0b2fe85a8d10d64c670866271235b0a5187e7015
parent: 0221ca95fc5829a9fd7d5348507ba2be0dea8264
author: Timothy B. Terriberry <tterribe@xiph.org>
date: Wed Dec 30 00:23:32 EST 2015 

Fix potential memory leaks with OpusServerInfo.

In op_[v]open_url() and op_[v]test_url(), if we successfully
 connected to the URL but fail to parse it as an Opus stream, then
 we would return to the calling application without clearing any
 OpusServerInfo we might have filled in when connecting.
This contradicts the general contract for user output buffers in
 our APIs, which is that they do not need to be initialized prior
 to a call and that their contents are untouched if a function
 fails (so that an application need do no additional clean-up on
 error).
It would have been possible for an application to avoid these leaks
 by always calling opus_server_info_init() before a call to
 op_[v]open_url() or op_[v]test_url() and always calling
 opus_server_info_clear() afterwards (even on failure), but our
 examples don't do this and no other API of ours requires it.

Fix the potential leaks by wrapping the implementation of
 op_url_stream_vcreate() so we can a) tell if the information was
 requested and b) store it in a separate, local buffer and delay
 copying it to the application until we know we've succeeded.


--- a/src/http.c
+++ b/src/http.c
@@ -3273,8 +3273,22 @@
 #endif
 }
 
-void *op_url_stream_vcreate(OpusFileCallbacks *_cb,
- const char *_url,va_list _ap){
+/*The actual implementation of op_url_stream_vcreate().
+  We have to do a careful dance here to avoid potential memory leaks if
+   OpusServerInfo is requested, since this function is also used by
+   op_vopen_url() and op_vtest_url().
+  Even if this function succeeds, those functions might ultimately fail.
+  If they do, they should return without having touched the OpusServerInfo
+   passed by the application.
+  Therefore, if this function succeeds and OpusServerInfo is requested, the
+   actual info will be stored in *_info and a pointer to the application's
+   storage will be placed in *_pinfo.
+  If this function fails or if the application did not request OpusServerInfo,
+   *_pinfo will be NULL.
+  Our caller is responsible for copying *_info to **_pinfo if it ultimately
+   succeeds, or for clearing *_info if it ultimately fails.*/
+void *op_url_stream_vcreate_impl(OpusFileCallbacks *_cb,
+ const char *_url,OpusServerInfo *_info,OpusServerInfo **_pinfo,va_list _ap){
   int             skip_certificate_check;
   const char     *proxy_host;
   opus_int32      proxy_port;
@@ -3318,14 +3332,14 @@
   }
   /*If the caller has requested server information, proxy it to a local copy to
      simplify error handling.*/
+  *_pinfo=NULL;
   if(pinfo!=NULL){
-    OpusServerInfo  info;
-    void           *ret;
-    opus_server_info_init(&info);
+    void *ret;
+    opus_server_info_init(_info);
     ret=op_url_stream_create_impl(_cb,_url,skip_certificate_check,
-     proxy_host,proxy_port,proxy_user,proxy_pass,&info);
-    if(ret!=NULL)*pinfo=*&info;
-    else opus_server_info_clear(&info);
+     proxy_host,proxy_port,proxy_user,proxy_pass,_info);
+    if(ret!=NULL)*_pinfo=pinfo;
+    else opus_server_info_clear(_info);
     return ret;
   }
   return op_url_stream_create_impl(_cb,_url,skip_certificate_check,
@@ -3332,6 +3346,16 @@
    proxy_host,proxy_port,proxy_user,proxy_pass,NULL);
 }
 
+void *op_url_stream_vcreate(OpusFileCallbacks *_cb,
+ const char *_url,va_list _ap){
+  OpusServerInfo   info;
+  OpusServerInfo *pinfo;
+  void *ret;
+  ret=op_url_stream_vcreate_impl(_cb,_url,&info,&pinfo,_ap);
+  if(pinfo!=NULL)*pinfo=*&info;
+  return ret;
+}
+
 void *op_url_stream_create(OpusFileCallbacks *_cb,
  const char *_url,...){
   va_list  ap;
@@ -3347,14 +3371,21 @@
 OggOpusFile *op_vopen_url(const char *_url,int *_error,va_list _ap){
   OpusFileCallbacks  cb;
   OggOpusFile       *of;
+  OpusServerInfo     info;
+  OpusServerInfo    *pinfo;
   void              *source;
-  source=op_url_stream_vcreate(&cb,_url,_ap);
+  source=op_url_stream_vcreate_impl(&cb,_url,&info,&pinfo,_ap);
   if(OP_UNLIKELY(source==NULL)){
+    OP_ASSERT(pinfo==NULL);
     if(_error!=NULL)*_error=OP_EFAULT;
     return NULL;
   }
   of=op_open_callbacks(source,&cb,NULL,0,_error);
-  if(OP_UNLIKELY(of==NULL))(*cb.close)(source);
+  if(OP_UNLIKELY(of==NULL)){
+    if(pinfo!=NULL)opus_server_info_clear(&info);
+    (*cb.close)(source);
+  }
+  else if(pinfo!=NULL)*pinfo=*&info;
   return of;
 }
 
@@ -3370,14 +3401,21 @@
 OggOpusFile *op_vtest_url(const char *_url,int *_error,va_list _ap){
   OpusFileCallbacks  cb;
   OggOpusFile       *of;
+  OpusServerInfo     info;
+  OpusServerInfo    *pinfo;
   void              *source;
-  source=op_url_stream_vcreate(&cb,_url,_ap);
+  source=op_url_stream_vcreate_impl(&cb,_url,&info,&pinfo,_ap);
   if(OP_UNLIKELY(source==NULL)){
+    OP_ASSERT(pinfo==NULL);
     if(_error!=NULL)*_error=OP_EFAULT;
     return NULL;
   }
   of=op_test_callbacks(source,&cb,NULL,0,_error);
-  if(OP_UNLIKELY(of==NULL))(*cb.close)(source);
+  if(OP_UNLIKELY(of==NULL)){
+    if(pinfo!=NULL)opus_server_info_clear(&info);
+    (*cb.close)(source);
+  }
+  else if(pinfo!=NULL)*pinfo=*&info;
   return of;
 }