Andrew Talbot wrote:
Changelog: itss: Replace malloc() with HeapAlloc().
diff --git a/dlls/itss/lzx.c b/dlls/itss/lzx.c index b5fdfc7..0ffeeb6 100644 --- a/dlls/itss/lzx.c +++ b/dlls/itss/lzx.c @@ -36,15 +36,17 @@ ***************************************************************************/
#include "lzx.h" +#include <stdarg.h> #include <stdio.h> #include <stdlib.h> #include <string.h>
+#include "windef.h" +#include "winbase.h"
/* sized types */ typedef unsigned char UBYTE; /* 8 bits exactly */ typedef unsigned short UWORD; /* 16 bits (or more) */ -typedef unsigned int ULONG; /* 32 bits (or more) */ -typedef signed int LONG; /* 32 bits (or more) */
/* some constants defined by the LZX specification */ #define LZX_MIN_MATCH (2) @@ -178,8 +180,8 @@ struct LZXstate *LZXinit(int window) if (window < 15 || window > 21) return NULL;
/* allocate state and associated window */
- pState = malloc(sizeof(struct LZXstate));
- if (!(pState->window = malloc(wndsize)))
- pState = HeapAlloc(GetProcessHeap(), 0, sizeof(struct LZXstate));
- if (!(pState->window = HeapAlloc(GetProcessHeap(), 0, wndsize))) { free(pState); return NULL;
Why didn't you replace corresponding free() calls?
On Tue, 10 Feb 2009, Nikolay Sivov wrote: [...]
Why didn't you replace corresponding free() calls?
Also, for everyone's information, there's more calls to malloc() and free(). I only grepped in the dlls/ directory where they are the most suspicious. I removed some obvious false positives but there may be more. Here's what I got:
dlls/itss/lzx.c:181: pState = malloc(sizeof(struct LZXstate)); dlls/itss/lzx.c:182: if (!(pState->window = malloc(wndsize))) dlls/rsaenh/mpi.c:157: a->dp = malloc (sizeof (mp_digit) * MP_PREC); dlls/rsaenh/mpi.c:185: a->dp = malloc (sizeof (mp_digit) * size); dlls/rsaenh/mpi.c:3381: tmp = malloc(bsize); dlls/twain_32/dsm_ctrl.c:89: devices = malloc(sizeof(devices[0])); dlls/winedos/dosconf.c:267: DOSCONF_config.country = malloc(strlen(*confline) + 1); dlls/winedos/dosconf.c:307: DOSCONF_config.shell = malloc(strlen(*confline) + 1); dlls/winedos/dosconf.c:370: DOSCONF_menu_default = malloc(strlen(*confline) + 1); dlls/winedos/dosconf.c:387: temp = malloc(strlen(*confline) + 1); dlls/winedos/dosvm.c:260: event = malloc(sizeof(DOSEVENT)); dlls/winedos/ppdev.c:184: PPDeviceList[nports].devicename = malloc(sizeof(buffer)+1); dlls/winemp3.acm/interface.c:74: nbuf = malloc( sizeof(struct buf) ); dlls/winemp3.acm/interface.c:79: nbuf->pnt = malloc(size); dlls/winenas.drv/audio.c:1383: void *newbuf = malloc(wwo->BufferUsed + len); dlls/winenas.drv/audio.c:1418: ptr = malloc(len); dlls/winenas.drv/audio.c:1420: newdata = malloc(wwo->BufferUsed - len); dlls/winhttp/net.c:634: * SSL 0.9.7 and above malloc the buffer if it is null. dlls/winhttp/net.c:653: if (malloc) free( buffer );
dlls/ddraw/tests/ddrawmodes.c:95: modes = malloc((modes_size = 2) * sizeof(DDSURFACEDESC)); dlls/dsound/tests/ds3d.c:51: b=buf=malloc(*size); dlls/lz32/tests/lzexpand_main.c:884: buf = malloc(uncompressed_data_size * 2); dlls/oleaut32/tests/vartype.c:35:# include <malloc.h> dlls/rpcrt4/tests/server.c:1202: dc = malloc(FIELD_OFFSET(doub_carr_t, a[2])); dlls/rpcrt4/tests/server.c:1204: dc->a[0] = malloc(FIELD_OFFSET(doub_carr_1_t, a[3])); dlls/rpcrt4/tests/server.c:1209: dc->a[1] = malloc(FIELD_OFFSET(doub_carr_1_t, a[2])); dlls/rpcrt4/tests/server.c:56: return malloc(n); dlls/shell32/tests/shlexec.c:336: cmd=malloc(strlen(argv0)+10+strlen(child_file)+2+strlen(cmdtail)+1);
dlls/dbghelp/msc.c:2480: free(files_image); dlls/itss/lzx.c:184: free(pState); dlls/itss/lzx.c:220: free(pState->window); dlls/itss/lzx.c:221: free(pState); dlls/ntdll/server.c:752: free( tmp_dir ); dlls/rsaenh/mpi.c:217: free(a->dp); dlls/rsaenh/mpi.c:3446: free(tmp); dlls/winedos/dosconf.c:357: free(DOSCONF_menu_default); dlls/winedos/dosconf.c:390: free(temp); dlls/winedos/dosvm.c:185: free(event); dlls/winedos/dosvm.c:625: free(event); dlls/winedos/int33.c:221: free(data); dlls/winejack.drv/audio.c:2006: free(ports); /* free the returned array of ports */ dlls/winejack.drv/audio.c:735: free(ports); /* free the returned array of ports */ dlls/winemp3.acm/interface.c:115: free(buf->pnt); dlls/winemp3.acm/interface.c:116: free(buf); dlls/winemp3.acm/interface.c:60: free(b->pnt); dlls/winemp3.acm/interface.c:62: free(b); dlls/winemp3.acm/interface.c:81: free(nbuf); dlls/winenas.drv/audio.c:1387: free(oldbuf); dlls/winenas.drv/audio.c:1432: free(ptr); dlls/wininet/netconnection.c:753: free(buffer);
dlls/ddraw/tests/ddrawmodes.c:104: free(modes); dlls/dsound/tests/ds3d8.c:521: free(state.wave); dlls/dsound/tests/ds3d.c:677: free(state.wave); dlls/lz32/tests/lzexpand_main.c:889: free(buf); dlls/ntdll/tests/rtlstr.c:1367: free(wstr); dlls/ntdll/tests/rtlstr.c:1407: free(wstr); dlls/ntdll/tests/rtlstr.c:296: free(teststring2); dlls/rpcrt4/tests/server.c:1214: free(dc->a[0]); dlls/rpcrt4/tests/server.c:1215: free(dc->a[1]); dlls/rpcrt4/tests/server.c:1216: free(dc); dlls/rpcrt4/tests/server.c:62: free(p); dlls/shell32/tests/shlexec.c:340: free(cmd);
Notes: * It's probably not as bad to use malloc() in tests but it should still be avoided. * Using #include <malloc.h> when not using malloc is wrong. * Some calls to malloc() and free() may be required by the Unix APIs that are called. This should be checked on a case by case basis. * If the above don't have too many false positives I could integrate that check into my scripts.
Francois Gouget wrote:
Also, for everyone's information, there's more calls to malloc() and free().
There are also many calls to realloc() - a function with complexities of its own - and other functions with "realloc" in their name.
On Tue, 10 Feb 2009, Andrew Talbot wrote: [...]
There are also many calls to realloc() - a function with complexities of its own - and other functions with "realloc" in their name.
Good point. Here are the ones I noticed:
dlls/rsaenh/mpi.c:3668: if ((tmp = realloc (a->dp, sizeof (mp_digit) * a->used)) == NULL) { dlls/rsaenh/mpi.c:72: tmp = realloc (a->dp, sizeof (mp_digit) * size); dlls/twain_32/dsm_ctrl.c:87: devices = realloc(devices, sizeof(devices[0])*(nrdevices+1));
dlls/advapi32/tests/registry.c:55: if ((ret = realloc( list[idx], size ))) list[idx] = ret; dlls/ddraw/tests/ddrawmodes.c:97: modes = realloc(modes, (modes_size *= 2) * sizeof(DDSURFACEDESC)); dlls/mstask/tests/task.c:41: if ((ret = realloc(list[idx], size))) dlls/oleaut32/tests/tmarshal.c:59: if ((ret = realloc( list[idx], size ))) list[idx] = ret; dlls/userenv/tests/userenv.c:54: if ((ret = realloc( list[idx], size ))) list[idx] = ret;
And we even have two calls to calloc()! dlls/winedos/int33.c:267: MCALLDATA *data = calloc(1,sizeof(MCALLDATA)); dlls/winedos/devices.c:137: hdr_ptr = calloc(1,sizeof(void *)+extra);
Francois Gouget wrote:
On Tue, 10 Feb 2009, Andrew Talbot wrote: [...]
There are also many calls to realloc() - a function with complexities of its own - and other functions with "realloc" in their name.
Good point. Here are the ones I noticed:
dlls/rsaenh/mpi.c:3668: if ((tmp = realloc (a->dp, sizeof (mp_digit) * a->used)) == NULL) { dlls/rsaenh/mpi.c:72: tmp = realloc (a->dp, sizeof (mp_digit) * size); dlls/twain_32/dsm_ctrl.c:87: devices = realloc(devices, sizeof(devices[0])*(nrdevices+1));
dlls/advapi32/tests/registry.c:55: if ((ret = realloc( list[idx], size ))) list[idx] = ret; dlls/ddraw/tests/ddrawmodes.c:97: modes = realloc(modes, (modes_size *= 2) * sizeof(DDSURFACEDESC)); dlls/mstask/tests/task.c:41: if ((ret = realloc(list[idx], size))) dlls/oleaut32/tests/tmarshal.c:59: if ((ret = realloc( list[idx], size ))) list[idx] = ret; dlls/userenv/tests/userenv.c:54: if ((ret = realloc( list[idx], size ))) list[idx] = ret;
And we even have two calls to calloc()! dlls/winedos/int33.c:267: MCALLDATA *data = calloc(1,sizeof(MCALLDATA)); dlls/winedos/devices.c:137: hdr_ptr = calloc(1,sizeof(void *)+extra);
Don't forget few places with strdup:
dlls/gphoto2.ds/gphoto2_main.c:70: gpfile->folder = strdup(folder); dlls/gphoto2.ds/gphoto2_main.c:71: gpfile->filename = strdup(name); dlls/twain_32/dsm_ctrl.c:90: devices[nrdevices].modname = strdup(dsname); dlls/wineps.drv/mkagl.c:141: glyphs[num_glyphs].name = strdup(namebuf); dlls/wineps.drv/mkagl.c:142: glyphs[num_glyphs].comment = strdup(commbuf); dlls/wineps.drv/mkagl.c:280: glyphs[num_glyphs].name = strdup(namebuf); dlls/wineps.drv/mkagl.c:281: glyphs[num_glyphs].comment = strdup(linebuf); dlls/winex11.drv/opengl.c:320: WineGLInfo.glExtensions = strdup((const char *) pglGetString(GL_EXTENSIONS));
Vitaliy.
On Wed, 11 Feb 2009, Vitaliy Margolen wrote: [...]
Don't forget few places with strdup:
Yes I've seen them too :-( Should we ban it too? What's the proper replacement? StrDupA() is only exported by slwapi.dll which means it's not a good choice...
-
Andrew Talbot -
Francois Gouget -
Nikolay Sivov -
Vitaliy Margolen