[vlc-devel] 2 weeks to 0.9.0 features freeze

[Rafaël Carré]

Le Mon, 2 Jun 2008 21:09:25 +0200,
Pierre d'Herbemont <pdherbemont at free.fr> a écrit :

> 
> On Jun 2, 2008, at 9:06 PM, Matthias Bauer wrote:

> > The patch was reviewed by Funman and Jean-Paul-Saman and I
> > implemented most of their change requests, but didn't get any
> > answer to my later posts. That disencouraged me to work on. I you
> > say yes, I would be pleased to continue working on this plugin.

I have a last few remarks:

+#define CARD8  uint8_t  
+#define CARD16 uint16_t
+#define CARD32 uint32_t  

What about putting them in the .h that needs them ? (after including
the correct header, which is inttypes.h)

+    int           i_alpha;             /* alpha transparency value */

That should be uint8_t , or whatever unsigned type which suits the
maximum value i_alpha can take

+   +    bool    b_vnc_poll;         /* Activatie VNC polling ? */
 bool    b_vnc_poll;         /* Activatie VNC polling ? */

Is "Activatie" german or just a typo ? ;)

+    p_sys->psz_host = var_CreateGetString( p_this, RMTOSD_CFG "host" );
+    if( EMPTY_STR(p_sys->psz_host) )

You shall free psz_host since it may be 1 byte allocated: '\0'

+    p_sys->psz_passwd = var_CreateGetString( p_this, RMTOSD_CFG
"password" );
+    if( !p_sys->psz_passwd )

You shall free psz_host & psz_passwd

+    p_sys->p_vout = vlc_object_find( p_this, VLC_OBJECT_VOUT,
FIND_ANYWHERE );

This should use FIND_PARENT, since the video_output (if any) we are
looking for is the creator of the remote_osd object, and not another
one which may be completely unrelated to us.

+    if( p_sys->psz_host ) free( p_sys->psz_host );
+
+    if( p_sys ) free( p_sys );

You don't need to check for the pointers being != NULL before free()ing
them.
You are also leaking p_sys->psz_passwd

+    FREENULL( p_sys->psz_host );
+    FREENULL( p_sys->psz_passwd );

Since you are closing the module, and freeing p_sys just the line
after, no need to use the FREENULL macro, free() is fine.

+    if ( i_bytes == net_Read( p_filter, i_socket, NULL,
+                              (unsigned char*)p_readbuf,
+                              i_bytes, true ) )
+        return true;
+    else 
+        return false;

You can use:
    return net_Read( p_filter, i_socket, NULL,
                               (unsigned char*)p_readbuf,
                               i_bytes, true ) );

Any integer != 0 means true, and 0 means false.

(same for write_exact() )

+    rfbProtocolVersionMsg pv;
+    pv[sz_rfbProtocolVersionMsg] = '\0';

It looks to me like an off-by-one overflow, shouldn't this be
sz_rfbProtocolVersionMsg-1 ?


+    if( i_nameLength > 100 )
+    {
+        msg_Err( p_filter, "Server name too long" );
+        return false;
+    }

Is 100 a limit fixed by yourself ?
If yes, you should use a #define for it, so people can change it
easily, without looking at all places where it is used.

+    while (p_sys->b_continue)
+    {
+       msg.type = 0;
+       if ( !read_exact(p_filter, p_sys->i_socket, (char*)&msg, 1 ) )

You rely on p_sys->b_continue to stop execution of other threads, but
in the close function you use:

+    free( p_sys );


That means in the while loop of a thread, such an execution could occur:

Check p_sys->b_continue: true, then execute the whole while loop
* Another thread is executed, which will call the close function
	p_sys is freed by the close function
* The while loop continues to be executed:
	if ( !read_exact(p_filter, p_sys->i_socket, (char*)&msg, 1 ) )

But now you are accessing the p_sys content which has been freed, and
is now invalid.

Proper synchronisation between all threads should be achieved with
mutexes.

+               i_msgSize--;
+               if (i_msgSize > 0)

You can use if( --i_msgSize > 0 ), that just makes code shorter ;)

+    filter_sys_t *p_sys = p_filter->p_sys;
+    subpicture_t *p_spu;
+    subpicture_region_t *p_region;
+    video_format_t fmt;
+    picture_t *p_pic;

If you declare the variables just before using them, it makes it a lot
better for people looking at your code in the future.


+    int i_line_offset = i_y * i_pitch + i_x;
+
+    for( int i_column = 0; i_column < i_w; i_column++ )
+    {
+        int i_offset = i_line_offset + i_column;
+        uint8_t i_color = p_sys->read_buffer[i_column]; 
+        p_outY->p_pixels[ i_offset ] =
p_sys->ar_color_table_yuv[i_color][0];

Are you sure i_offset can not overflow p_outY->p_pixels' size here ?


+    if( i_y < 0 || i_x < 0 || i_y >= v_h || i_x >= v_w )
+        return VLC_SUCCESS;

Might need a msg_Dbg() here, no ?

+    /* msg_Dbg( p_this, "mouse event x=%d y=%d btn=%x", i_x, i_y,
i_v ); */

You should use #ifdef VNC_DEBUG, no ?

+    /* Fixme: remove this "handmade" DES crypt algoritm */

When you send the next patch, I will have a look at the problem with
libgcrypt, so keep it like it for now, and focus on the threads
synchronization ;)

If I can not find the cause, I will commit it as is (with the
"handmade" DES algo), so it can be tested by the community.


Sorry for the delay, the patch is quite long to review.
We should be near the end now, thanks for your patience.

-- 
Rafaël Carré
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20080602/13679bec/attachment.sig>