[vlc-devel] [PATCH] vlc_input: fix memcpy to NULL or uninitialized memory and clean up code

[Quink]

On Sat, Dec 12, 2015 at 06:54:15PM +0200, R�mi Denis-Courmont wrote:
> On Sunday 13 December 2015 00:38:05 Zhao Zhili wrote:
> > From: Zhao Zhili <wantlamy at gmail.com>
> > 
> > ---
> >  include/vlc_input.h | 23 +++++++++--------------
> >  1 file changed, 9 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/vlc_input.h b/include/vlc_input.h
> > index f6209e3..bf890cc 100644
> > --- a/include/vlc_input.h
> > +++ b/include/vlc_input.h
> > @@ -102,16 +102,7 @@ typedef struct input_title_t
> > 
> >  static inline input_title_t *vlc_input_title_New(void)
> >  {
> > -    input_title_t *t = (input_title_t*)malloc( sizeof( input_title_t ) );
> > -    if( !t )
> > -        return NULL;
> > -
> > -    t->psz_name = NULL;
> > -    t->i_flags = 0;
> > -    t->i_length = 0;
> > -    t->i_seekpoint = 0;
> > -    t->seekpoint = NULL;
> 
> I fail to see an improvement here. calloc()/memset() is not strictly warranted 
> to set pointer to NULL. So this is just an uglier way to get the same result.
> 
Yes you are right. Some code in vlc replaced malloc() by calloc() and I
learned in a wrong way. Is it right to use compound literal to
initialize a struct?

diff --git a/include/vlc_input.h b/include/vlc_input.h
index fbe7f79..8469e0f 100644
--- a/include/vlc_input.h
+++ b/include/vlc_input.h
@@ -106,12 +106,7 @@ static inline input_title_t *vlc_input_title_New(void)
     if( !t )
         return NULL;
 
-    t->psz_name = NULL;
-    t->i_flags = 0;
-    t->i_length = 0;
-    t->i_seekpoint = 0;
-    t->seekpoint = NULL;
-
+    *t = (input_title_t){NULL, 0, 0, 0, NULL};
     return t;
 }
 
This is still ugly but if I let
+    *t = (input_title_t){0};
gcc will throw uninitialized warning.

> > -
> > +    input_title_t *t = (input_title_t*)calloc( 1, sizeof( input_title_t )
> > );
> >      return t;
> >  }
> > 
> > @@ -189,9 +180,13 @@ static inline input_attachment_t
> > *vlc_input_attachment_New( const char *psz_name
> >      a->psz_mime = strdup( psz_mime ? psz_mime : "" );
> >      a->psz_description = strdup( psz_description ? psz_description : "" );
> >      a->i_data = i_data;
> > -    a->p_data = malloc( i_data );
> > -    if( i_data > 0 && likely(p_data != NULL) )
> 
> Indeed, this was wrong as it checks p_data instead of a->p_data.
> But you are not fixing it properly.

A new patch is appended.

> 
> > -        memcpy( a->p_data, p_data, i_data );
> > +    a->p_data = NULL;
> > +    if( i_data > 0 && likely(p_data) )
> 
> Please don´t remove explicit NULL comparisons.
> 
> The compiler can optimize it if it wants to, so there is no point.
> 
> > +    {
> > +        a->p_data = malloc( i_data );
> > +        if( likely(a->p_data) )
> > +            memcpy( a->p_data, p_data, i_data );
> > +    }
> > 
> >      if( unlikely(a->psz_name == NULL || a->psz_mime == NULL
> > 
> >                || a->psz_description == NULL || (i_data > 0 && a->p_data ==
> > 
> > NULL)) )
> > @@ -233,7 +228,7 @@ struct input_thread_t
> >      bool b_preparsing;
> >      bool b_dead VLC_DEPRECATED;
> > 
> > -    /* All other data is input_thread is PRIVATE. You can't access it
> > +    /* All other data in input_thread_t is PRIVATE. You can't access it
> >       * outside of src/input */
> >      input_thread_private_t *p;
> >  };
> 
> -- 
> Rémi Denis-Courmont
> http://www.remlab.net/
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

vlc_input: check malloc return

---
 include/vlc_input.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/vlc_input.h b/include/vlc_input.h
index f6209e3..fbe7f79 100644
--- a/include/vlc_input.h
+++ b/include/vlc_input.h
@@ -190,7 +190,7 @@ static inline input_attachment_t *vlc_input_attachment_New( const char *psz_name
     a->psz_description = strdup( psz_description ? psz_description : "" );
     a->i_data = i_data;
     a->p_data = malloc( i_data );
-    if( i_data > 0 && likely(p_data != NULL) )
+    if( i_data > 0 && likely(a->p_data != NULL) )
         memcpy( a->p_data, p_data, i_data );
 
     if( unlikely(a->psz_name == NULL || a->psz_mime == NULL
-- 
1.9.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-vlc_input-check-malloc-return.patch
Type: text/x-diff
Size: 870 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20151213/12dd216c/attachment.patch>