Thanks Dok....
On Thu, 2006-06-29 at 18:25 +0200, Denis Oliver Kropp wrote:
> kandagatla.Srinivas wrote:
> > Hi All,
> >
> > There is a major Bug in DirectFB-0.9.25.1which will crash DFB.
>
> Thanks for spotting this.
>
> > Look at the below code..from(system/fbdev/fbdev.c)
> >
> > shared->cmap_memory = SHMALLOC( pool_data, 256 * 2 * 12 );
> > shared->orig_cmap.start = 0;
> > shared->orig_cmap.len = 256;
> > shared->orig_cmap.red = shared->cmap_memory + 256 * 2 * 0;
> > shared->orig_cmap.green = shared->cmap_memory + 256 * 2 * 1;
> > shared->orig_cmap.blue = shared->cmap_memory + 256 * 2 * 2;
> > shared->orig_cmap.transp = shared->cmap_memory + 256 * 2 * 3;
> >
> > if (ioctl( dfb_fbdev->fd, FBIOGETCMAP, &shared->orig_cmap ) < 0) {
> > D_PERROR( "DirectFB/FBDev: "
> > "Could not retrieve palette for backup!\n" );
> > SHFREE( pool_data, shared->cmap_memory );
>
> The easiest fix would be to simply not free the memory here,
> but saving 2k is a good idea :)
>
I think we should go by saving of 2k Mem. and making the pointer to
NULL.
> > The below patch fixes the above discussed problem.
> > Mail me if you want more detail..
> >
> > diff -urN 1/systems/fbdev/fbdev.c 2/systems/fbdev/fbdev.c
> > --- 1/systems/fbdev/fbdev.c 2006-06-29 10:09:34.000000000 +0530
> > +++ 2/systems/fbdev/fbdev.c 2006-05-03 12:52:37.000000000 +0530
> > @@ -508,36 +508,33 @@
> > dfb_fbdev_var_to_mode( &shared->current_var,
> > &shared->current_mode );
> >
> > -/* shared->cmap_memory = SHMALLOC( pool_data, 256 * 2 * 12 );*/
> > - shared->orig_cmap_memory = SHMALLOC( pool_data, 256 * 2 * 4 );
> > + shared->cmap_memory = SHMALLOC( pool_data, 256 * 2 * 12 );
>
> Took a few seconds to recognize that your patch is reversed :)
Sorry for that...
>
> > shared->orig_cmap.start = 0;
> > shared->orig_cmap.len = 256;
> > - shared->orig_cmap.red = shared->orig_cmap_memory + 256 * 2 * 0;
> > - shared->orig_cmap.green = shared->orig_cmap_memory + 256 * 2 * 1;
> > - shared->orig_cmap.blue = shared->orig_cmap_memory + 256 * 2 * 2;
> > - shared->orig_cmap.transp = shared->orig_cmap_memory + 256 * 2 * 3;
> > + shared->orig_cmap.red = shared->cmap_memory + 256 * 2 * 0;
> > + shared->orig_cmap.green = shared->cmap_memory + 256 * 2 * 1;
> > + shared->orig_cmap.blue = shared->cmap_memory + 256 * 2 * 2;
> > + shared->orig_cmap.transp = shared->cmap_memory + 256 * 2 * 3;
> >
> > if (ioctl( dfb_fbdev->fd, FBIOGETCMAP, &shared->orig_cmap ) < 0) {
> > D_PERROR( "DirectFB/FBDev: "
> > "Could not retrieve palette for backup!\n" );
> > - SHFREE( pool_data, shared->orig_cmap_memory );
> > + SHFREE( pool_data, shared->cmap_memory );
> > shared->orig_cmap.len = 0;
>
> Maybe it's even better to memset the cmap struct with zero to also NULL
> all the pointers and protect against accidental usage later on.
>
Ya this will be the good idea to set the mem to 0 when we create the
struture instance only..
> > }
> >
> > - shared->temp_cmap_memory = SHMALLOC( pool_data, 256 * 2 * 4 );
> > shared->temp_cmap.len = 256;
> > - shared->temp_cmap.red = shared->temp_cmap_memory + 256 * 2 * 4;
> > - shared->temp_cmap.green = shared->temp_cmap_memory + 256 * 2 * 5;
> > - shared->temp_cmap.blue = shared->temp_cmap_memory + 256 * 2 * 6;
> > - shared->temp_cmap.transp = shared->temp_cmap_memory + 256 * 2 * 7;
> > + shared->temp_cmap.red = shared->cmap_memory + 256 * 2 * 4;
> > + shared->temp_cmap.green = shared->cmap_memory + 256 * 2 * 5;
> > + shared->temp_cmap.blue = shared->cmap_memory + 256 * 2 * 6;
> > + shared->temp_cmap.transp = shared->cmap_memory + 256 * 2 * 7;
>
> You forgot to change the offsets from 4,5,6,7 to 0,1,2,3 :)
>
> > - shared->current_cmap_memory = SHMALLOC( pool_data, 256 * 2 * 4 );
> > shared->current_cmap.len = 256;
> > - shared->current_cmap.red = shared->current_cmap_memory + 256 *
> > 2 * 8;
> > - shared->current_cmap.green = shared->current_cmap_memory + 256 *
> > 2 * 9;
> > - shared->current_cmap.blue = shared->current_cmap_memory + 256 *
> > 2 * 10;
> > - shared->current_cmap.transp = shared->current_cmap_memory + 256 *
> > 2 * 11;
> > + shared->current_cmap.red = shared->cmap_memory + 256 * 2 * 8;
> > + shared->current_cmap.green = shared->cmap_memory + 256 * 2 * 9;
> > + shared->current_cmap.blue = shared->cmap_memory + 256 * 2 * 10;
> > + shared->current_cmap.transp = shared->cmap_memory + 256 * 2 * 11;
>
> dito
>
> > dfb_fbdev_get_pci_info( shared );
> >
> > @@ -667,12 +664,7 @@
> > "Could not restore palette!\n" );
> > }
> >
> > - if( shared->orig_cmap.len> 0) /* Check if memory IS allocated*/
> > - SHFREE( shared->shmpool_data, shared->orig_cmap_memory );
> > - if( shared->current_cmap.len> 0)/* Check if memory IS allocated*/
> > - SHFREE( shared->shmpool_data, shared->current_cmap_memory );
> > - if( shared->temp_cmap.len> 0)/* Check if memory IS allocated*/
> > - SHFREE( shared->shmpool_data, shared->orig_cmap_memory );
>
> Better keep pointers NULL if memory is not allocated and check for that
> only.
I belive this function gets only called when we do the core shutdown..
or when the diretFB core closes.
but your point is valid for other SHFREE's...
>
>
>
> By that opportunity, we should add checks after SHMALLOC() to do
> D_OOSHM() if it returns NULL :-)
Sounds Intresting....
>
Regards
Srinivas.Kandagatla
Celunite SoftSystems.
Hyderbad.
_______________________________________________
directfb-dev mailing list
[email protected]
http://mail.directfb.org/cgi-bin/mailman/listinfo/directfb-dev
