| Summary: | memory issue | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Arvin Mittal <arvin.mittal> |
| Component: | video | Assignee: | Sam Lantinga <slouken> |
| Status: | RESOLVED DUPLICATE | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | CC: | arvin.mittal, arvinmittal1, philipp.wiesemann |
| Version: | 1.2.11 | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: | Modified file with fix for memory issue. | ||
Seems to be a duplicate of bug 2299 but the patch is different there. 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 *** (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 (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. (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. :) (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...:) (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). |
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); } /*******************************************************************/