We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 2334 - memory issue
Summary: memory issue
Status: RESOLVED DUPLICATE of bug 2299
Alias: None
Product: SDL
Classification: Unclassified
Component: video (show other bugs)
Version: 1.2.11
Hardware: All All
: P2 normal
Assignee: Sam Lantinga
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-03 09:44 UTC by Arvin Mittal
Modified: 2014-11-03 18:22 UTC (History)
3 users (show)

See Also:

Attachments
Modified file with fix for memory issue. (40.79 KB, text/plain)
2014-01-03 09:44 UTC, Arvin Mittal 		Details
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Arvin Mittal 2014-01-03 09:44:54 UTC 
Created attachment 1512 [details]
Modified file with fix for memory issue.

There was memory leak in SDL_CreateYUV_SW function in SDL_yuv_sw.c file.

Existing code:
/*******************************************************************/
overlay = (SDL_Overlay *)SDL_malloc(sizeof *overlay);
	if ( overlay == NULL ) {
		SDL_OutOfMemory();
		return(NULL);
	}
	SDL_memset(overlay, 0, (sizeof *overlay));

	/* Fill in the basic members */
	overlay->format = format;
	overlay->w = width;
	overlay->h = height;

	/* Set up the YUV surface function structure */
	overlay->hwfuncs = &sw_yuvfuncs;

	/* Create the pixel data and lookup tables */
	swdata = (struct private_yuvhwdata *)SDL_malloc(sizeof *swdata);
	overlay->hwdata = swdata;
	if ( swdata == NULL ) {
		SDL_OutOfMemory();
		SDL_FreeYUVOverlay(overlay);
		return(NULL);
	}
	swdata->stretch = NULL;
	swdata->display = display;
	swdata->pixels = (Uint8 *) SDL_malloc(width*height*2);
	swdata->colortab = (int *)SDL_malloc(4*256*sizeof(int));
	Cr_r_tab = &swdata->colortab[0*256];
	Cr_g_tab = &swdata->colortab[1*256];
	Cb_g_tab = &swdata->colortab[2*256];
	Cb_b_tab = &swdata->colortab[3*256];
	swdata->rgb_2_pix = (Uint32 *)SDL_malloc(3*768*sizeof(Uint32));
	r_2_pix_alloc = &swdata->rgb_2_pix[0*768];
	g_2_pix_alloc = &swdata->rgb_2_pix[1*768];
	b_2_pix_alloc = &swdata->rgb_2_pix[2*768];
	if ( ! swdata->pixels || ! swdata->colortab || ! swdata->rgb_2_pix ) {
		SDL_OutOfMemory();
		SDL_FreeYUVOverlay(overlay);
		return(NULL);
	}
/*******************************************************************/

In above code if memory allocation to any of swdata->pixels, swdata->colortab and  swdata->rgb_2_pix fails, current code is not freeing memory of pointers allocated before them(example if  memory allocation to swdata->rgb_2_pix gets failed, then memory of swdata->pixels, swdata->colortab and swdata is not freed), before actually being return, and there will be memory leak.


Modified code:
In modified code, memory allocation is checked at each step, and if memory allocation get failed, then every memory allocated prior to current allocation is freed and program returns safely.

/*******************************************************************/
overlay = (SDL_Overlay *)SDL_malloc(sizeof *overlay);
	if ( overlay == NULL ) {
		SDL_OutOfMemory();
		return(NULL);
	}
	SDL_memset(overlay, 0, (sizeof *overlay));

	/* Fill in the basic members */
	overlay->format = format;
	overlay->w = width;
	overlay->h = height;

	/* Set up the YUV surface function structure */
	overlay->hwfuncs = &sw_yuvfuncs;

	/* Create the pixel data and lookup tables */
	swdata = (struct private_yuvhwdata *)SDL_malloc(sizeof *swdata);
	overlay->hwdata = swdata;
	if ( swdata == NULL ) {
		SDL_OutOfMemory();
		SDL_FreeYUVOverlay(overlay);
		return(NULL);
	}
	swdata->stretch = NULL;
	swdata->display = display;
	swdata->pixels = (Uint8 *) SDL_malloc(width*height*2);
	if ( swdata->pixels == NULL ) {
		SDL_OutOfMemory();
		SDL_free(swdata);
		SDL_FreeYUVOverlay(overlay);
		return(NULL);
	}
	swdata->colortab = (int *)SDL_malloc(4*256*sizeof(int));
	if ( swdata->colortab == NULL ) {
		SDL_OutOfMemory();
		SDL_free(swdata->pixels);
		SDL_free(swdata);
		SDL_FreeYUVOverlay(overlay);
		return(NULL);
	}
	Cr_r_tab = &swdata->colortab[0*256];
	Cr_g_tab = &swdata->colortab[1*256];
	Cb_g_tab = &swdata->colortab[2*256];
	Cb_b_tab = &swdata->colortab[3*256];
	swdata->rgb_2_pix = (Uint32 *)SDL_malloc(3*768*sizeof(Uint32));
	if ( swdata->rgb_2_pix == NULL ) {
		SDL_OutOfMemory();
		SDL_free(swdata->colortab);
		SDL_free(swdata->pixels);
		SDL_free(swdata);
		SDL_FreeYUVOverlay(overlay);
		return(NULL);
	}
/*******************************************************************/
Comment 1 Philipp Wiesemann 2014-10-29 20:10:48 UTC 
Seems to be a duplicate of bug 2299 but the patch is different there.
Comment 2 Philipp Wiesemann 2014-11-01 12:44:20 UTC 
Bug 2299 supplies a similar patch and has been filed earlier. It also has has a more descriptive title.

*** This bug has been marked as a duplicate of bug 2299 ***
Comment 3 Arvin 2014-11-03 03:33:02 UTC 
(In reply to Philipp Wiesemann from comment #2)
> Bug 2299 supplies a similar patch and has been filed earlier. It also has
> has a more descriptive title.

*** This bug has been marked as a duplicate
> of bug 2299 ***

I think the patch described in bug 2299 does not fully cover the memory loss, there is still memory leakage left.
Please check again.
Thanks
Comment 4 Arvin 2014-11-03 03:40:12 UTC 
(In reply to Philipp Wiesemann from comment #2)
> Bug 2299 supplies a similar patch and has been filed earlier. It also has
> has a more descriptive title.

*** This bug has been marked as a duplicate
> of bug 2299 ***

Hi Philipp
Patch described in 2299, doesn't free pointers (swdata->colortab, swdata->pixels)
whereas my patch covers all the issues and has been tested fine. 
please check.
Comment 5 Philipp Wiesemann 2014-11-03 08:57:35 UTC 
(In reply to Arvin from comment #4)
> (In reply to Philipp Wiesemann from comment #2)
> > Bug 2299 supplies a similar patch and has been filed earlier. It also has
> > has a more descriptive title.
> 
> *** This bug has been marked as a duplicate
> > of bug 2299 ***
> 
> Hi Philipp
> Patch described in 2299, doesn't free pointers (swdata->colortab,
> swdata->pixels)
> whereas my patch covers all the issues and has been tested fine. 
> please check.

As far as I can tell the two pointers would also be freed with the other patch. The only functional difference I can see is that this patch here additionally frees swdata. But maybe I am missing something.

I think that both patches free too much because SDL_FreeYUVOverlay() would do the same cleanup (without double free). I added a comment about this in bug 2299. (Maybe I should have closed one bug before adding that comment which would have been less confusing because the related mail would have been send to all. :)
Comment 6 Arvin 2014-11-03 09:35:26 UTC 
(In reply to Philipp Wiesemann from comment #5)
> (In reply to Arvin from comment #4)
> (In reply to Philipp Wiesemann from
> comment #2)
> > Bug 2299 supplies a similar patch and has been filed
> earlier. It also has
> > has a more descriptive title.
> 
> *** This bug has
> been marked as a duplicate
> > of bug 2299 ***
> 
> Hi Philipp
> Patch
> described in 2299, doesn't free pointers (swdata->colortab,
>
> swdata->pixels)
> whereas my patch covers all the issues and has been tested
> fine. 
> please check.

As far as I can tell the two pointers would also be
> freed with the other patch. The only functional difference I can see is that
> this patch here additionally frees swdata. But maybe I am missing something.
> I think that both patches free too much because SDL_FreeYUVOverlay() would
> do the same cleanup (without double free). I added a comment about this in
> bug 2299. (Maybe I should have closed one bug before adding that comment
> which would have been less confusing because the related mail would have
> been send to all. :)

With other patch, it will be case of dangling pointers, which is more dangerous.
First these pointers(swdata->colortab and swdata->pixels) are to be freed, then swdata, and then overlay. Otherwise if there is error in allocating memory to swdata->rgb_2_pix, and we free directly overlay, then it will create dangling pointers, and these pointers(swdata->colortab and swdata->pixels) will never be reached again. SDL_FreeYUVOverlay() will free only overlay created by SDL_CreateYUVOverlay and will not free other pointers.
Please check...:)
Comment 7 Philipp Wiesemann 2014-11-03 18:22:45 UTC 
(In reply to Arvin from comment #6)
> ...
> swdata->pixels) will never be reached again. SDL_FreeYUVOverlay() will free
> only overlay created by SDL_CreateYUVOverlay and will not free other
> pointers.
> ...

SDL_FreeYUVOverlay() calls the function pointer FreeHW stored in the hwfuncs structure of the SDL_Overlay structure. The hwfuncs structure is filled in SDL_CreateYUV_SW(). This means for FreeHW the function SDL_FreeYUV_SW() is called. And SDL_FreeYUV_SW() frees all these pointers (it is at the end of SDL_yuv_sw.c).