From 0c33d3a6483e2493a29d662afc79a8c86dc6dfc5 Mon Sep 17 00:00:00 2001 From: simon Date: Mon, 22 Jul 2013 07:11:54 +0000 Subject: [PATCH] Another big batch of memory leak fixes, again mostly on error paths. The most interesting one is printer_add_enum, which I've modified to take a char ** rather than a char * so that it can both realloc its input buffer _and_ return NULL to indicate error. git-svn-id: svn://svn.tartarus.org/sgt/putty@9959 cda61777-01e9-0310-a592-d414129be87e --- windows/winctrls.c | 6 +++++- windows/window.c | 15 ++++++++++++--- windows/winjump.c | 8 ++++++-- windows/winpgen.c | 12 +++++++----- windows/winpgnt.c | 42 +++++++++++++++++++++++++++++++++--------- windows/winpgntc.c | 6 +++++- windows/winprint.c | 25 +++++++++++-------------- windows/winproxy.c | 2 ++ windows/winstore.c | 33 ++++++++++++++++++++++++++------- 9 files changed, 107 insertions(+), 42 deletions(-) diff --git a/windows/winctrls.c b/windows/winctrls.c index 60e32f5e..49c13e8d 100644 --- a/windows/winctrls.c +++ b/windows/winctrls.c @@ -448,6 +448,8 @@ char *staticwrap(struct ctlpos *cp, HWND hwnd, char *text, int *lines) if (lines) *lines = nlines; + sfree(pwidths); + return ret; } @@ -1665,7 +1667,9 @@ void winctrl_layout(struct dlgparam *dp, struct winctrls *wc, winctrl_add_shortcuts(dp, c); if (actual_base_id == base_id) base_id += num_ids; - } + } else { + sfree(data); + } if (colstart >= 0) { /* diff --git a/windows/window.c b/windows/window.c index b2a5c5cb..b477d6ae 100644 --- a/windows/window.c +++ b/windows/window.c @@ -2197,8 +2197,10 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, reconfig_result = do_reconfig(hwnd, back ? back->cfg_info(backhandle) : 0); reconfiguring = FALSE; - if (!reconfig_result) + if (!reconfig_result) { + conf_free(prev_conf); break; + } conf_cache_data(); @@ -4903,10 +4905,17 @@ void write_clip(void *frontend, wchar_t * data, int *attr, int len, int must_des GlobalFree(clipdata2); return; } - if (!(lock = GlobalLock(clipdata))) + if (!(lock = GlobalLock(clipdata))) { + GlobalFree(clipdata); + GlobalFree(clipdata2); return; - if (!(lock2 = GlobalLock(clipdata2))) + } + if (!(lock2 = GlobalLock(clipdata2))) { + GlobalUnlock(clipdata); + GlobalFree(clipdata); + GlobalFree(clipdata2); return; + } memcpy(lock, data, len * sizeof(wchar_t)); WideCharToMultiByte(CP_ACP, 0, data, len, lock2, len2, NULL, NULL); diff --git a/windows/winjump.c b/windows/winjump.c index 563d42cd..abefe445 100644 --- a/windows/winjump.c +++ b/windows/winjump.c @@ -410,16 +410,20 @@ static IShellLink *make_shell_link(const char *appname, /* Check if this is a valid session, otherwise don't add. */ if (sessionname) { psettings_tmp = open_settings_r(sessionname); - if (!psettings_tmp) + if (!psettings_tmp) { + sfree(app_path); return NULL; + } close_settings_r(psettings_tmp); } /* Create the new item. */ if (!SUCCEEDED(CoCreateInstance(&CLSID_ShellLink, NULL, CLSCTX_INPROC_SERVER, - COMPTR(IShellLink, &ret)))) + COMPTR(IShellLink, &ret)))) { + sfree(app_path); return NULL; + } /* Set path, parameters, icon and description. */ ret->lpVtbl->SetPath(ret, app_path); diff --git a/windows/winpgen.c b/windows/winpgen.c index aeebb629..9fa35af0 100644 --- a/windows/winpgen.c +++ b/windows/winpgen.c @@ -420,11 +420,11 @@ static int save_ssh1_pubkey(char *filename, struct RSAKey *key) char *dec1, *dec2; FILE *fp; - dec1 = bignum_decimal(key->exponent); - dec2 = bignum_decimal(key->modulus); fp = fopen(filename, "wb"); if (!fp) return 0; + dec1 = bignum_decimal(key->exponent); + dec2 = bignum_decimal(key->modulus); fprintf(fp, "%d %s %s %s\n", bignum_bitcount(key->modulus), dec1, dec2, key->comment); fclose(fp); @@ -1256,9 +1256,11 @@ static int CALLBACK MainDlgProc(HWND hwnd, UINT msg, if (!state->generation_thread_exists) { char filename[FILENAME_MAX]; if (prompt_keyfile(hwnd, "Load private key:", - filename, 0, LOWORD(wParam)==IDC_LOAD)) - load_key_file(hwnd, state, filename_from_str(filename), - LOWORD(wParam) != IDC_LOAD); + filename, 0, LOWORD(wParam)==IDC_LOAD)) { + Filename *fn = filename_from_str(filename); + load_key_file(hwnd, state, fn, LOWORD(wParam) != IDC_LOAD); + filename_free(fn); + } } break; } diff --git a/windows/winpgnt.c b/windows/winpgnt.c index e220d6bb..1f6484f9 100644 --- a/windows/winpgnt.c +++ b/windows/winpgnt.c @@ -811,8 +811,10 @@ static void *get_keylist1(int *length) retval = agent_query(request, 5, &vresponse, &resplen, NULL, NULL); assert(retval == 1); response = vresponse; - if (resplen < 5 || response[4] != SSH1_AGENT_RSA_IDENTITIES_ANSWER) + if (resplen < 5 || response[4] != SSH1_AGENT_RSA_IDENTITIES_ANSWER) { + sfree(response); return NULL; + } ret = snewn(resplen-5, unsigned char); memcpy(ret, response+5, resplen-5); @@ -846,8 +848,10 @@ static void *get_keylist2(int *length) retval = agent_query(request, 5, &vresponse, &resplen, NULL, NULL); assert(retval == 1); response = vresponse; - if (resplen < 5 || response[4] != SSH2_AGENT_IDENTITIES_ANSWER) + if (resplen < 5 || response[4] != SSH2_AGENT_IDENTITIES_ANSWER) { + sfree(response); return NULL; + } ret = snewn(resplen-5, unsigned char); memcpy(ret, response+5, resplen-5); @@ -942,12 +946,17 @@ static void answer_msg(void *msg) goto failure; p += i; i = ssh1_read_bignum(p, msgend - p, &reqkey.modulus); - if (i < 0) + if (i < 0) { + freebn(reqkey.exponent); goto failure; + } p += i; i = ssh1_read_bignum(p, msgend - p, &challenge); - if (i < 0) + if (i < 0) { + freebn(reqkey.exponent); + freebn(reqkey.modulus); goto failure; + } p += i; if (msgend < p+16) { freebn(reqkey.exponent); @@ -1437,10 +1446,12 @@ static void prompt_add_keyfile(void) of.lpstrTitle = "Select Private Key File"; of.Flags = OFN_ALLOWMULTISELECT | OFN_EXPLORER; if (request_file(keypath, &of, TRUE, FALSE)) { - if(strlen(filelist) > of.nFileOffset) + if(strlen(filelist) > of.nFileOffset) { /* Only one filename returned? */ - add_keyfile(filename_from_str(filelist)); - else { + Filename *fn = filename_from_str(filelist); + add_keyfile(fn); + filename_free(fn); + } else { /* we are returned a bunch of strings, end to * end. first string is the directory, the * rest the filenames. terminated with an @@ -1450,7 +1461,9 @@ static void prompt_add_keyfile(void) char *filewalker = filelist + strlen(dir) + 1; while (*filewalker != '\0') { char *filename = dupcat(dir, "\\", filewalker, NULL); - add_keyfile(filename_from_str(filename)); + Filename *fn = filename_from_str(filename); + add_keyfile(fn); + filename_free(fn); sfree(filename); filewalker += strlen(filewalker) + 1; } @@ -1908,6 +1921,7 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, #ifdef DEBUG_IPC debug(("couldn't get user SID\n")); #endif + CloseHandle(filemap); return 0; } @@ -1915,6 +1929,8 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, #ifdef DEBUG_IPC debug(("couldn't get default SID\n")); #endif + CloseHandle(filemap); + sfree(ourself); return 0; } @@ -1926,6 +1942,9 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, debug(("couldn't get owner info for filemap: %d\n", rc)); #endif + CloseHandle(filemap); + sfree(ourself); + sfree(ourself2); return 0; } #ifdef DEBUG_IPC @@ -1944,6 +1963,9 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, if (!EqualSid(mapowner, ourself) && !EqualSid(mapowner, ourself2)) { CloseHandle(filemap); + LocalFree(psd); + sfree(ourself); + sfree(ourself2); return 0; /* security ID mismatch! */ } #ifdef DEBUG_IPC @@ -2127,7 +2149,9 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show) command = ""; break; } else { - add_keyfile(filename_from_str(argv[i])); + Filename *fn = filename_from_str(argv[i]); + add_keyfile(fn); + filename_free(fn); added_keys = TRUE; } } diff --git a/windows/winpgntc.c b/windows/winpgntc.c index 4074a165..7b8b5093 100644 --- a/windows/winpgntc.c +++ b/windows/winpgntc.c @@ -209,8 +209,10 @@ int agent_query(void *in, int inlen, void **out, int *outlen, filemap = CreateFileMapping(INVALID_HANDLE_VALUE, psa, PAGE_READWRITE, 0, AGENT_MAX_MSGLEN, mapname); - if (filemap == NULL || filemap == INVALID_HANDLE_VALUE) + if (filemap == NULL || filemap == INVALID_HANDLE_VALUE) { + sfree(mapname); return 1; /* *out == NULL, so failure */ + } p = MapViewOfFile(filemap, FILE_MAP_WRITE, 0, 0, 0); memcpy(p, in, inlen); cds.dwData = AGENT_COPYDATA_ID; @@ -237,6 +239,7 @@ int agent_query(void *in, int inlen, void **out, int *outlen, data->hwnd = hwnd; if (CreateThread(NULL, 0, agent_query_thread, data, 0, &threadid)) return 0; + sfree(mapname); sfree(data); } #endif @@ -258,6 +261,7 @@ int agent_query(void *in, int inlen, void **out, int *outlen, } UnmapViewOfFile(p); CloseHandle(filemap); + sfree(mapname); if (psd) LocalFree(psd); sfree(usersid); diff --git a/windows/winprint.c b/windows/winprint.c index 1548c3c9..bc5a5d12 100644 --- a/windows/winprint.c +++ b/windows/winprint.c @@ -18,39 +18,39 @@ struct printer_job_tag { HANDLE hprinter; }; -static char *printer_add_enum(int param, DWORD level, char *buffer, - int offset, int *nprinters_ptr) +static int printer_add_enum(int param, DWORD level, char **buffer, + int offset, int *nprinters_ptr) { DWORD needed = 0, nprinters = 0; - buffer = sresize(buffer, offset+512, char); + *buffer = sresize(*buffer, offset+512, char); /* * Exploratory call to EnumPrinters to determine how much space * we'll need for the output. Discard the return value since it * will almost certainly be a failure due to lack of space. */ - EnumPrinters(param, NULL, level, buffer+offset, 512, + EnumPrinters(param, NULL, level, (*buffer)+offset, 512, &needed, &nprinters); if (needed < 512) needed = 512; - buffer = sresize(buffer, offset+needed, char); + *buffer = sresize(*buffer, offset+needed, char); - if (EnumPrinters(param, NULL, level, buffer+offset, + if (EnumPrinters(param, NULL, level, (*buffer)+offset, needed, &needed, &nprinters) == 0) - return NULL; + return FALSE; *nprinters_ptr += nprinters; - return buffer; + return TRUE; } printer_enum *printer_start_enum(int *nprinters_ptr) { printer_enum *ret = snew(printer_enum); - char *buffer = NULL, *retval; + char *buffer = NULL; *nprinters_ptr = 0; /* default return value */ buffer = snewn(512, char); @@ -71,12 +71,9 @@ printer_enum *printer_start_enum(int *nprinters_ptr) ret->enum_level = 4; } - retval = printer_add_enum(PRINTER_ENUM_LOCAL | PRINTER_ENUM_CONNECTIONS, - ret->enum_level, buffer, 0, nprinters_ptr); - if (!retval) + if (!printer_add_enum(PRINTER_ENUM_LOCAL | PRINTER_ENUM_CONNECTIONS, + ret->enum_level, &buffer, 0, nprinters_ptr)) goto error; - else - buffer = retval; switch (ret->enum_level) { case 4: diff --git a/windows/winproxy.c b/windows/winproxy.c index 75a78529..7bbd18f0 100644 --- a/windows/winproxy.c +++ b/windows/winproxy.c @@ -180,6 +180,7 @@ Socket platform_new_connection(SockAddr addr, char *hostname, sa.bInheritHandle = TRUE; if (!CreatePipe(&us_from_cmd, &cmd_to_us, &sa, 0)) { ret->error = dupprintf("Unable to create pipes for proxy command"); + sfree(cmd); return (Socket)ret; } @@ -187,6 +188,7 @@ Socket platform_new_connection(SockAddr addr, char *hostname, CloseHandle(us_from_cmd); CloseHandle(cmd_to_us); ret->error = dupprintf("Unable to create pipes for proxy command"); + sfree(cmd); return (Socket)ret; } diff --git a/windows/winstore.c b/windows/winstore.c index 66c7c759..47fef678 100644 --- a/windows/winstore.c +++ b/windows/winstore.c @@ -167,7 +167,10 @@ char *read_setting_s(void *handle, const char *key) ret = snewn(size+1, char); if (RegQueryValueEx((HKEY) handle, key, 0, &type, ret, &size) != ERROR_SUCCESS || - type != REG_SZ) return NULL; + type != REG_SZ) { + sfree(ret); + return NULL; + } return ret; } @@ -190,6 +193,7 @@ FontSpec *read_setting_fontspec(void *handle, const char *name) { char *settingname; char *fontname; + FontSpec *ret; int isbold, height, charset; fontname = read_setting_s(handle, name); @@ -199,19 +203,30 @@ FontSpec *read_setting_fontspec(void *handle, const char *name) settingname = dupcat(name, "IsBold", NULL); isbold = read_setting_i(handle, settingname, -1); sfree(settingname); - if (isbold == -1) return NULL; + if (isbold == -1) { + sfree(fontname); + return NULL; + } settingname = dupcat(name, "CharSet", NULL); charset = read_setting_i(handle, settingname, -1); sfree(settingname); - if (charset == -1) return NULL; + if (charset == -1) { + sfree(fontname); + return NULL; + } settingname = dupcat(name, "Height", NULL); height = read_setting_i(handle, settingname, INT_MIN); sfree(settingname); - if (height == INT_MIN) return NULL; + if (height == INT_MIN) { + sfree(fontname); + return NULL; + } - return fontspec_new(fontname, isbold, height, charset); + ret = fontspec_new(fontname, isbold, height, charset); + sfree(fontname); + return ret; } void write_setting_fontspec(void *handle, const char *name, FontSpec *font) @@ -340,16 +355,18 @@ int verify_host_key(const char *hostname, int port, * Now read a saved key in from the registry and see what it * says. */ - otherstr = snewn(len, char); regname = snewn(3 * (strlen(hostname) + strlen(keytype)) + 15, char); hostkey_regname(regname, hostname, port, keytype); if (RegOpenKey(HKEY_CURRENT_USER, PUTTY_REG_POS "\\SshHostKeys", - &rkey) != ERROR_SUCCESS) + &rkey) != ERROR_SUCCESS) { + sfree(regname); return 1; /* key does not exist in registry */ + } readlen = len; + otherstr = snewn(len, char); ret = RegQueryValueEx(rkey, regname, NULL, &type, otherstr, &readlen); if (ret != ERROR_SUCCESS && ret != ERROR_MORE_DATA && @@ -412,6 +429,8 @@ int verify_host_key(const char *hostname, int port, RegSetValueEx(rkey, regname, 0, REG_SZ, otherstr, strlen(otherstr) + 1); } + + sfree(oldstyle); } RegCloseKey(rkey); -- 2.11.0