Fix unchecked strdups

* add missing checks * adapt function return values where necessary * add initial test for settings
yurashek · Jun 22, 2015 · bf73f4e · bf73f4e
1 parent 9fab504
commit bf73f4e
Show file tree

Hide file tree

Showing 63 changed files with 1,402 additions and 542 deletions.
diff --git a/client/Windows/cli/wfreerdp.c b/client/Windows/cli/wfreerdp.c
@@ -50,6 +50,7 @@ INT WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine
 	rdpContext* context;
 	rdpSettings* settings;
 	RDP_CLIENT_ENTRY_POINTS clientEntryPoints;
+	int ret = 0;
 
 	ZeroMemory(&clientEntryPoints, sizeof(RDP_CLIENT_ENTRY_POINTS));
 	clientEntryPoints.Size = sizeof(RDP_CLIENT_ENTRY_POINTS);
@@ -66,9 +67,24 @@ INT WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine
 
 	context->argc = __argc;
 	context->argv = (char**) malloc(sizeof(char*) * __argc);
+	if (!context->argv)
+	{
+		ret = 1;
+		goto out;
+	}
 
 	for (index = 0; index < context->argc; index++)
+	{
 		context->argv[index] = _strdup(__argv[index]);
+		if (!context->argv[index])
+		{
+			ret = 1;
+			free(context->argv);
+			context->argv = NULL;
+			goto out;
+		}
+
+	}
 
 	status = freerdp_client_settings_parse_command_line(settings, context->argc, context->argv, FALSE);
 
@@ -89,8 +105,8 @@ INT WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine
 	GetExitCodeThread(thread, &dwExitCode);
 
 	freerdp_client_stop(context);
-
+out:
 	freerdp_client_context_free(context);
 
-	return 0;
+	return ret;
 }
diff --git a/client/Windows/wf_client.c b/client/Windows/wf_client.c
@@ -534,13 +534,31 @@ BOOL wf_authenticate(freerdp* instance, char** username, char** password, char**
 	status = CredUIParseUserNameA(UserName, User, sizeof(User), Domain, sizeof(Domain));
 	//WLog_ERR(TAG, "User: %s Domain: %s Password: %s", User, Domain, Password);
 	*username = _strdup(User);
+	if (!(*username))
+	{
+		WLog_ERR(TAG, "strdup failed", status);
+		return FALSE;
+	}
 
 	if (strlen(Domain) > 0)
 		*domain = _strdup(Domain);
 	else
 		*domain = _strdup("\0");
 
+	if (!(*domain))
+	{
+		free(*username);
+		WLog_ERR(TAG, "strdup failed", status);
+		return FALSE;
+	}
+
 	*password = _strdup(Password);
+	if (!(*password))
+	{
+		free(*username);
+		free(*domain);
+		return FALSE;
+	}
 
 	return TRUE;
 }
@@ -873,6 +891,10 @@ int freerdp_client_load_settings_from_rdp_file(wfContext* wfc, char* filename)
 	if (filename)
 	{
 		settings->ConnectionFile = _strdup(filename);
+		if (!settings->ConnectionFile)
+		{
+			return 3;
+		}
 
 		// free old settings file
 		freerdp_client_rdp_file_free(wfc->connectionRdpFile);

diff --git a/client/X11/xf_client.c b/client/X11/xf_client.c
@@ -552,6 +552,8 @@ BOOL xf_create_window(xfContext* xfc)
 		if (settings->WindowTitle)
 		{
 			windowTitle = _strdup(settings->WindowTitle);
+			if (!windowTitle)
+				return FALSE;
 		}
 		else if (settings->ServerPort == 3389)
 		{
@@ -999,6 +1001,8 @@ BOOL xf_pre_connect(freerdp* instance)
 		if (login_name)
 		{
 			settings->Username = _strdup(login_name);
+			if (!settings->Username)
+				return FALSE;
 			WLog_INFO(TAG, "No user name set. - Using login name: %s", settings->Username);
 		}
 	}
@@ -1018,7 +1022,8 @@ BOOL xf_pre_connect(freerdp* instance)
 	if (!context->cache)
 		context->cache = cache_new(settings);
 
-	xf_keyboard_init(xfc);
+	if (!xf_keyboard_init(xfc))
+		return FALSE;
 
 	xf_detect_monitors(xfc, &maxWidth, &maxHeight);
 

diff --git a/client/X11/xf_cliprdr.c b/client/X11/xf_cliprdr.c
@@ -836,6 +836,16 @@ static int xf_cliprdr_server_format_list(CliprdrClientContext* context, CLIPRDR_
 		format = &formatList->formats[i];
 		clipboard->serverFormats[i].formatId = format->formatId;
 		clipboard->serverFormats[i].formatName = _strdup(format->formatName);
+		if (!clipboard->serverFormats[i].formatName)
+		{
+			for (--i; i >= 0; --i)
+				free(clipboard->serverFormats[i].formatName);
+
+			clipboard->numServerFormats = 0;
+			free(clipboard->serverFormats);
+			clipboard->serverFormats = NULL;
+			return -1;
+		}
 	}
 
 	clipboard->numTargets = 2;
@@ -1103,6 +1113,12 @@ xfClipboard* xf_clipboard_new(xfContext* xfc)
 	clipboard->clientFormats[n].atom = XInternAtom(xfc->display, "text/html", False);
 	clipboard->clientFormats[n].formatId = CB_FORMAT_HTML;
 	clipboard->clientFormats[n].formatName = _strdup("HTML Format");
+	if (!clipboard->clientFormats[n].formatName)
+	{
+		ClipboardDestroy(clipboard->system);
+		free(clipboard);
+		return NULL;
+	}
 	n++;
 
 	clipboard->numClientFormats = n;

diff --git a/client/X11/xf_event.c b/client/X11/xf_event.c
@@ -87,7 +87,6 @@ const char* const X11_EVENT_STRINGS[] =
 
 BOOL xf_event_action_script_init(xfContext* xfc)
 {
-	int exitCode;
 	char* xevent;
 	FILE* actionScript;
 	char buffer[1024] = { 0 };
@@ -102,18 +101,22 @@ BOOL xf_event_action_script_init(xfContext* xfc)
 
 	actionScript = popen(command, "r");
 
-	if (actionScript < 0)
+	if (actionScript <= 0)
 		return FALSE;
 
 	while (fgets(buffer, sizeof(buffer), actionScript))
 	{
 		strtok(buffer, "\n");
 		xevent = _strdup(buffer);
-		if (ArrayList_Add(xfc->xevents, xevent) < 0)
+		if (!xevent || ArrayList_Add(xfc->xevents, xevent) < 0)
+		{
+			ArrayList_Free(xfc->xevents);
+			xfc->xevents = NULL;
 			return FALSE;
+		}
 	}
 
-	exitCode = pclose(actionScript);
+	pclose(actionScript);
 
 	return TRUE;
 }
@@ -127,23 +130,22 @@ void xf_event_action_script_free(xfContext* xfc)
 	}
 }
 
-int xf_event_execute_action_script(xfContext* xfc, XEvent* event)
+static BOOL xf_event_execute_action_script(xfContext* xfc, XEvent* event)
 {
 	int index;
 	int count;
 	char* name;
-	int exitCode;
 	FILE* actionScript;
 	BOOL match = FALSE;
 	const char* xeventName;
 	char buffer[1024] = { 0 };
 	char command[1024] = { 0 };
 
-	if (!xfc->actionScript)
-		return 1;
+	if (!xfc->actionScript || !xfc->xevents)
+		return FALSE;
 
 	if (event->type > (sizeof(X11_EVENT_STRINGS) / sizeof(const char*)))
-		return 1;
+		return FALSE;
 
 	xeventName = X11_EVENT_STRINGS[event->type];
 
@@ -161,24 +163,24 @@ int xf_event_execute_action_script(xfContext* xfc, XEvent* event)
 	}
 
 	if (!match)
-		return 1;
+		return FALSE;
 
 	sprintf_s(command, sizeof(command), "%s xevent %s %d",
 			xfc->actionScript, xeventName, (int) xfc->window->handle);
 
 	actionScript = popen(command, "r");
 
 	if (actionScript < 0)
-		return -1;
+		return FALSE;
 
 	while (fgets(buffer, sizeof(buffer), actionScript))
 	{
 		strtok(buffer, "\n");
 	}
 
-	exitCode = pclose(actionScript);
+	pclose(actionScript);
 
-	return 1;
+	return TRUE;
 }
 
 void xf_event_adjust_coordinates(xfContext* xfc, int* x, int *y)

diff --git a/client/X11/xf_keyboard.c b/client/X11/xf_keyboard.c
@@ -61,11 +61,11 @@ BOOL xf_keyboard_action_script_init(xfContext* xfc)
 		xfc->actionScript = _strdup("/usr/share/freerdp/action.sh");
 
 	if (!xfc->actionScript)
-		return 0;
+		return FALSE;
 
 	xfc->keyCombinations = ArrayList_New(TRUE);
 	if (!xfc->keyCombinations)
-		return 0;
+		return FALSE;
 
 	ArrayList_Object(xfc->keyCombinations)->fnObjectFree = free;
 
@@ -77,20 +77,26 @@ BOOL xf_keyboard_action_script_init(xfContext* xfc)
 	{
 		free(xfc->actionScript);
 		xfc->actionScript = NULL;
-		return 0;
+		return FALSE;
 	}
 
 	while (fgets(buffer, sizeof(buffer), keyScript) != NULL)
 	{
 		strtok(buffer, "\n");
 		keyCombination = _strdup(buffer);
-		if (ArrayList_Add(xfc->keyCombinations, keyCombination) < 0)
-			return 0;
+		if (!keyCombination || ArrayList_Add(xfc->keyCombinations, keyCombination) < 0)
+		{
+			ArrayList_Free(xfc->keyCombinations);
+			free(xfc->actionScript);
+			xfc->actionScript = NULL;
+			pclose(keyScript);
+			return FALSE;
+		}
 	}
 
-	exitCode = pclose(keyScript);
-
+	pclose(keyScript);
 	return xf_event_action_script_init(xfc);
+
 }
 
 void xf_keyboard_action_script_free(xfContext* xfc)
@@ -110,7 +116,7 @@ void xf_keyboard_action_script_free(xfContext* xfc)
 	}
 }
 
-void xf_keyboard_init(xfContext* xfc)
+BOOL xf_keyboard_init(xfContext* xfc)
 {
 	xf_keyboard_clear(xfc);
 
@@ -121,9 +127,11 @@ void xf_keyboard_init(xfContext* xfc)
 	if (xfc->modifierMap)
 		XFreeModifiermap(xfc->modifierMap);
 
-	xfc->modifierMap = XGetModifierMapping(xfc->display);
+	if (!(xfc->modifierMap = XGetModifierMapping(xfc->display)))
+		return FALSE;
 
 	xf_keyboard_action_script_init(xfc);
+	return TRUE;
 }
 
 void xf_keyboard_free(xfContext* xfc)

diff --git a/client/X11/xf_keyboard.h b/client/X11/xf_keyboard.h
@@ -44,7 +44,7 @@ struct _XF_MODIFIER_KEYS
 };
 typedef struct _XF_MODIFIER_KEYS XF_MODIFIER_KEYS;
 
-void xf_keyboard_init(xfContext* xfc);
+BOOL xf_keyboard_init(xfContext* xfc);
 void xf_keyboard_free(xfContext* xfc);
 void xf_keyboard_clear(xfContext* xfc);
 void xf_keyboard_key_press(xfContext* xfc, BYTE keycode, KeySym keysym);

diff --git a/client/X11/xf_rail.c b/client/X11/xf_rail.c
@@ -312,6 +312,11 @@ static BOOL xf_rail_window_common(rdpContext* context, WINDOW_ORDER_INFO* orderI
 		{
 			appWindow->title = _strdup("RdpRailWindow");
 		}
+		if (!appWindow->title)
+		{
+			free(appWindow);
+			return FALSE;
+		}
 
 		HashTable_Add(xfc->railWindows, (void*) (UINT_PTR) orderInfo->windowId, (void*) appWindow);
 

diff --git a/client/common/client.c b/client/common/client.c
@@ -213,13 +213,20 @@ int freerdp_client_settings_parse_command_line(rdpSettings* settings, int argc,
 int freerdp_client_settings_parse_connection_file(rdpSettings* settings, const char* filename)
 {
 	rdpFile* file;
+	int ret = -1;
 
 	file = freerdp_client_rdp_file_new();
-	freerdp_client_parse_rdp_file(file, filename);
-	freerdp_client_populate_settings_from_rdp_file(file, settings);
-	freerdp_client_rdp_file_free(file);
+	if (!file)
+		return -1;
+	if (!freerdp_client_parse_rdp_file(file, filename))
+		goto out;
+	if (!freerdp_client_populate_settings_from_rdp_file(file, settings))
+		goto out;
 
-	return 0;
+	ret = 0;
+out:
+	freerdp_client_rdp_file_free(file);
+	return ret;
 }
 
 int freerdp_client_settings_parse_connection_file_buffer(rdpSettings* settings, const BYTE* buffer, size_t size)
@@ -228,6 +235,8 @@ int freerdp_client_settings_parse_connection_file_buffer(rdpSettings* settings,
 	int status = -1;
 
 	file = freerdp_client_rdp_file_new();
+	if (!file)
+		return -1;
 
 	if (freerdp_client_parse_rdp_file_buffer(file, buffer, size)
 			&& freerdp_client_populate_settings_from_rdp_file(file, settings))
@@ -243,18 +252,23 @@ int freerdp_client_settings_parse_connection_file_buffer(rdpSettings* settings,
 int freerdp_client_settings_write_connection_file(const rdpSettings* settings, const char* filename, BOOL unicode)
 {
 	rdpFile* file;
+	int ret = -1;
 
 	file = freerdp_client_rdp_file_new();
+	if (!file)
+		return -1;
 
 	if (!freerdp_client_populate_rdp_file_from_settings(file, settings))
-		return -1;
+		goto out;
 
 	if (!freerdp_client_write_rdp_file(file, filename, unicode))
-		return -1;
+		goto out;
 
+	ret = 0;
+out:
 	freerdp_client_rdp_file_free(file);
 
-	return 0;
+	return ret;
 }
 
 int freerdp_client_settings_parse_assistance_file(rdpSettings* settings, const char* filename)