MonetDB: Jul2015 - Try not to overflow snprintf buffers.

Ying Zhang Y.Zhang at cwi.nl
Fri Jul 24 23:21:50 CEST 2015


> On Jul 24, 2015, at 16:36, Sjoerd Mullender <commits at monetdb.org> wrote:
> 
> Changeset: c84b68960bca for MonetDB
> URL: http://dev.monetdb.org/hg/MonetDB?cmd=changeset;node=c84b68960bca
> Modified Files:
> 	clients/mapiclient/ReadlineTools.c
> 	clients/mapiclient/dump.c
> 	clients/mapilib/mapi.c
> 	clients/odbc/driver/SQLExecute.c
> 	common/options/monet_options.c
> 	common/stream/stream.c
> 	common/utils/mutils.c
> 	gdk/gdk_bbp.c
> Branch: Jul2015
> Log Message:
> 
> Try not to overflow snprintf buffers.
> 
> 
> diffs (truncated from 373 to 300 lines):
> 
> diff --git a/clients/mapiclient/ReadlineTools.c b/clients/mapiclient/ReadlineTools.c
> --- a/clients/mapiclient/ReadlineTools.c
> +++ b/clients/mapiclient/ReadlineTools.c
> @@ -61,12 +61,16 @@ sql_tablename_generator(const char *text
> 	static MapiHdl table_hdl;
> 
> 	if (!state) {
> -		char query[512];
> +		char *query;
> 
> 		seekpos = 0;
> 		len = strlen(text);
> -		snprintf(query, sizeof(query), "SELECT t.\"name\", s.\"name\" FROM \"sys\".\"tables\" t, \"sys\".\"schemas\" s where t.schema_id = s.id AND t.\"name\" like '%s%%'", text);
> -		if ((table_hdl = mapi_query(_mid, query)) == NULL || mapi_error(_mid)) {
> +		if ((query = malloc(len + 128)) == NULL)
> +			return NULL;
> +		snprintf(query, len + 128, "SELECT t.\"name\", s.\"name\" FROM \"sys\".\"tables\" t, \"sys\".\"schemas\" s where t.schema_id = s.id AND t.\"name\" like '%s%%'", text);
> +		table_hdl = mapi_query(_mid, query);
> +		free(query);
> +		if (table_hdl == NULL || mapi_error(_mid)) {
> 			if (table_hdl) {
> 				mapi_explain_query(table_hdl, stderr);
> 				mapi_close_handle(table_hdl);
> @@ -176,7 +180,7 @@ static char *mal_commands[] = {
> static int
> mal_help(int cnt, int key)
> {
> -	char *name, *c, buf[512];
> +	char *name, *c, *buf;
> 	int seekpos = 0, rowcount;
> 	MapiHdl table_hdl;
> 
> @@ -188,8 +192,12 @@ mal_help(int cnt, int key)
> 		c--;
> 	while (c > rl_line_buffer && !isspace(*c))
> 		c--;
> -	snprintf(buf, sizeof(buf), "manual.help(\"%s\");", c);
> -	if ((table_hdl = mapi_query(_mid, buf)) == NULL || mapi_error(_mid)) {
> +	if ((buf = malloc(strlen(c) + 20)) == NULL)
> +		return 0;
> +	snprintf(buf, strlen(c) + 20, "manual.help(\"%s\");", c);
> +	table_hdl = mapi_query(_mid, buf);
> +	free(buf);
> +	if (table_hdl == NULL || mapi_error(_mid)) {
> 		if (table_hdl) {
> 			mapi_explain_query(table_hdl, stderr);
> 			mapi_close_handle(table_hdl);
> @@ -220,7 +228,7 @@ mal_command_generator(const char *text, 
> 	static int idx;
> 	static int seekpos, len, rowcount;
> 	static MapiHdl table_hdl;
> -	char *name, buf[512];
> +	char *name, *buf;
> 
> 	/* we pick our own portion of the linebuffer */
> 	text = rl_line_buffer + strlen(rl_line_buffer) - 1;
> @@ -250,14 +258,18 @@ mal_command_generator(const char *text, 
> 			text = c + 2;
> 		while (isspace((int) *text))
> 			text++;
> +		if ((buf = malloc(strlen(text) + 32)) == NULL)
> +			return NULL;
> 		if (strchr(text, '.') == NULL)
> -			snprintf(buf, sizeof(buf),
> +			snprintf(buf, strlen(text) + 32,
> 				 "manual.completion(\"%s.*(\");", text);
> 		else
> -			snprintf(buf, sizeof(buf),
> +			snprintf(buf, strlen(text) + 32,
> 				 "manual.completion(\"%s(\");", text);
> 		seekpos = 0;
> -		if ((table_hdl = mapi_query(_mid, buf)) == NULL || mapi_error(_mid)) {
> +		table_hdl = mapi_query(_mid, buf);
> +		free(buf);
> +		if (table_hdl == NULL || mapi_error(_mid)) {
> 			if (table_hdl) {
> 				mapi_explain_query(table_hdl, stderr);
> 				mapi_close_handle(table_hdl);
> diff --git a/clients/mapiclient/dump.c b/clients/mapiclient/dump.c
> --- a/clients/mapiclient/dump.c
> +++ b/clients/mapiclient/dump.c
> @@ -212,7 +212,7 @@ dump_foreign_keys(Mapi mid, const char *
> 			       "fkt.name = '%s' "
> 			 "ORDER BY fkk.name, nr", schema, tname);
> 	} else if (tid != NULL) {
> -		maxquerylen = 1024;
> +		maxquerylen = 1024 + strlen(tid);
> 		query = malloc(maxquerylen);
> 		snprintf(query, maxquerylen,
> 			 "SELECT ps.name, "		/* 0 */
> @@ -529,6 +529,8 @@ dump_column_definition(Mapi mid, stream 
> 	maxquerylen = 1024;
> 	if (tid == NULL)
> 		maxquerylen += strlen(tname) + strlen(schema);
> +	else
> +		maxquerylen += strlen(tid);
> 	if ((query = malloc(maxquerylen)) == NULL)
> 		goto bailout;
> 
> diff --git a/clients/mapilib/mapi.c b/clients/mapilib/mapi.c
> --- a/clients/mapilib/mapi.c
> +++ b/clients/mapilib/mapi.c
> @@ -1918,7 +1918,7 @@ mapi_new(void)
> Mapi
> mapi_mapiuri(const char *url, const char *user, const char *pass, const char *lang)
> {
> -	char uri[8096];
> +	char *uri;
> 	char *host;
> 	int port;
> 	char *dbname;
> @@ -1961,12 +1961,13 @@ mapi_mapiuri(const char *url, const char
> 	}
> 
> 	/* copy to a writable working buffer */
> -	snprintf(uri, 8096, "%s", url + sizeof("mapi:monetdb://") - 1);
> +	uri = strdup(url + sizeof("mapi:monetdb://") - 1);
> 
> 	if (uri[0] != '/') {
> 		if ((p = strchr(uri, ':')) == NULL) {
> 			mapi_setError(mid, "URI must contain a port number after "
> 					"the hostname", "mapi_mapiuri", MERROR);
> +			free(uri);
> 			return mid;
> 		}
> 		*p++ = '\0';
> @@ -1990,6 +1991,7 @@ mapi_mapiuri(const char *url, const char
> 		if (port <= 0) {
> 			mapi_setError(mid, "URI contains invalid port",
> 					"mapi_mapiuri", MERROR);
> +			free(uri);
> 			return mid;
> 		}
> 	} else {
> @@ -2029,6 +2031,7 @@ mapi_mapiuri(const char *url, const char
> 		mid->database = strdup(dbname);
> 
> 	set_uri(mid);
> +	free(uri);
> 
> 	return mid;
> }
> @@ -2128,7 +2131,8 @@ mapi_mapi(const char *host, int port, co
> 				while ((e = readdir(d)) != NULL) {
> 					if (strncmp(e->d_name, ".s.monetdb.", 11) != 0)
> 						continue;
> -					snprintf(buf, sizeof(buf), "/tmp/%s", e->d_name);
> +					if (snprintf(buf, sizeof(buf), "/tmp/%s", e->d_name) >= sizeof(buf))

Hai Sjoerd,

This line triggers the compiler error:

/.../src/MonetDB-11.21/clients/mapilib/mapi.c:2134:59: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
      if (snprintf(buf, sizeof(buf), "/tmp/%s", e->d_name) >= sizeof(buf))

Jennie

> +						continue; /* ignore long name */
> 					if (stat(buf, &st) != -1 && S_ISSOCK(st.st_mode))
> 						socks[i++] = atoi(e->d_name + 11);
> 					if (i == sizeof(socks))
> @@ -2320,7 +2324,8 @@ parse_uri_query(Mapi mid, char *uri)
> static void
> set_uri(Mapi mid)
> {
> -	char uri[1024];
> +	size_t urilen = strlen(mid->hostname) + (mid->database ? strlen(mid->database) : 0) + 32;
> +	char *uri = malloc(urilen);
> 
> 	/* uri looks as follows:
> 	 *  mapi:monetdb://host:port/database
> @@ -2330,25 +2335,25 @@ set_uri(Mapi mid)
> 
> 	if (mid->database != NULL) {
> 		if (mid->hostname[0] == '/') {
> -			snprintf(uri, sizeof(uri), "mapi:monetdb://%s?database=%s",
> -					mid->hostname, mid->database);
> +			snprintf(uri, urilen, "mapi:monetdb://%s?database=%s",
> +				 mid->hostname, mid->database);
> 		} else {
> -			snprintf(uri, sizeof(uri), "mapi:monetdb://%s:%d/%s",
> -					mid->hostname, mid->port, mid->database);
> +			snprintf(uri, urilen, "mapi:monetdb://%s:%d/%s",
> +				 mid->hostname, mid->port, mid->database);
> 		}
> 	} else {
> 		if (mid->hostname[0] == '/') {
> -			snprintf(uri, sizeof(uri), "mapi:monetdb://%s",
> -					mid->hostname);
> +			snprintf(uri, urilen, "mapi:monetdb://%s",
> +				 mid->hostname);
> 		} else {
> -			snprintf(uri, sizeof(uri), "mapi:monetdb://%s:%d",
> -					mid->hostname, mid->port);
> +			snprintf(uri, urilen, "mapi:monetdb://%s:%d",
> +				 mid->hostname, mid->port);
> 		}
> 	}
> 
> 	if (mid->uri != NULL)
> 		free(mid->uri);
> -	mid->uri = strdup(uri);
> +	mid->uri = uri;
> }
> 
> /* (Re-)establish a connection with the server. */
> @@ -2657,7 +2662,7 @@ mapi_start_talking(Mapi mid)
> 				pwdhash = mcrypt_MD5Sum(mid->password,
> 						strlen(mid->password));
> 			} else {
> -				snprintf(buf, BLOCK, "server requires unknown hash '%s'",
> +				snprintf(buf, BLOCK, "server requires unknown hash '%.100s'",
> 						serverhash);
> 				close_connection(mid);
> 				return mapi_setError(mid, buf, "mapi_start_talking", MERROR);
> @@ -2688,7 +2693,7 @@ mapi_start_talking(Mapi mid)
> 		}
> 		if (hash == NULL) {
> 			/* the server doesn't support what we can */
> -			snprintf(buf, BLOCK, "unsupported hash algorithms: %s", hashes);
> +			snprintf(buf, BLOCK, "unsupported hash algorithms: %.100s", hashes);
> 			close_connection(mid);
> 			return mapi_setError(mid, buf, "mapi_start_talking", MERROR);
> 		}
> @@ -2697,14 +2702,18 @@ mapi_start_talking(Mapi mid)
> 
> 		/* note: if we make the database field an empty string, it
> 		 * means we want the default.  However, it *should* be there. */
> -		snprintf(buf, BLOCK, "%s:%s:%s:%s:%s:\n",
> +		if (snprintf(buf, BLOCK, "%s:%s:%s:%s:%s:\n",
> #ifdef WORDS_BIGENDIAN
> -			 "BIG",
> +			     "BIG",
> #else
> -			 "LIT",
> +			     "LIT",
> #endif
> -			 mid->username, hash, mid->language,
> -			 mid->database == NULL ? "" : mid->database);
> +			     mid->username, hash, mid->language,
> +			     mid->database == NULL ? "" : mid->database) >= BLOCK) {;
> +			mapi_setError(mid, "combination of database name and user name too long", "mapi_start_talking", MERROR);
> +			free(hash);
> +			return mid->error;
> +		}
> 
> 		free(hash);
> 	} else {
> @@ -2873,7 +2882,7 @@ mapi_start_talking(Mapi mid)
> 			} else {
> 				char re[BUFSIZ];
> 				snprintf(re, sizeof(re),
> -						"error while parsing redirect: %s\n", red);
> +					 "error while parsing redirect: %.100s\n", red);
> 				mapi_close_handle(hdl);
> 				mapi_setError(mid, re, "mapi_start_talking", MERROR);
> 				return mid->error;
> diff --git a/clients/odbc/driver/SQLExecute.c b/clients/odbc/driver/SQLExecute.c
> --- a/clients/odbc/driver/SQLExecute.c
> +++ b/clients/odbc/driver/SQLExecute.c
> @@ -443,8 +443,7 @@ MNDBExecute(ODBCStmt *stmt)
> 		addStmtError(stmt, "HY001", NULL, 0);
> 		return SQL_ERROR;
> 	}
> -	snprintf(query, querylen, "execute %d (", stmt->queryid);
> -	querypos = strlen(query);
> +	querypos = snprintf(query, querylen, "execute %d (", stmt->queryid);
> 	/* XXX fill in parameter values */
> 	if (desc->sql_desc_bind_offset_ptr)
> 		offset = *desc->sql_desc_bind_offset_ptr;
> diff --git a/common/options/monet_options.c b/common/options/monet_options.c
> --- a/common/options/monet_options.c
> +++ b/common/options/monet_options.c
> @@ -216,7 +216,6 @@ mo_builtin_settings(opt **Set)
> {
> 	int i = 0;
> 	opt *set;
> -	char buf[BUFSIZ];
> 
> 	if (Set == NULL)
> 		return 0;
> @@ -228,10 +227,8 @@ mo_builtin_settings(opt **Set)
> 
> 	set[i].kind = opt_builtin;
> 	set[i].name = strdup("gdk_dbpath");
> -	snprintf(buf, BUFSIZ, "%s%c%s%c%s%c%s",
> -		 LOCALSTATEDIR, DIR_SEP, "monetdb5", DIR_SEP, "dbfarm",
> -		 DIR_SEP, "demo");
> -	set[i].value = strdup(buf);
> +	set[i].value = strdup(LOCALSTATEDIR DIR_SEP_STR "monetdb5" DIR_SEP_STR
> +			      "dbfarm" DIR_SEP_STR "demo");
> 	i++;
> 	set[i].kind = opt_builtin;
> 	set[i].name = strdup("gdk_debug");
> diff --git a/common/stream/stream.c b/common/stream/stream.c
> --- a/common/stream/stream.c
> +++ b/common/stream/stream.c
> @@ -510,16 +510,19 @@ error(stream *s)
> 
> 	switch (s->errnr) {
> 	case MNSTR_OPEN_ERROR:
> -		snprintf(buf, sizeof(buf), "error could not open file %s\n", s->name);
> +		snprintf(buf, sizeof(buf), "error could not open file %.100s\n", 
> +			 s->name);
> 		return strdup(buf);
> 	case MNSTR_READ_ERROR:
> -		snprintf(buf, sizeof(buf), "error reading file %s\n", s->name);
> +		snprintf(buf, sizeof(buf), "error reading file %.100s\n",
> +			 s->name);
> _______________________________________________
> checkin-list mailing list
> checkin-list at monetdb.org
> https://www.monetdb.org/mailman/listinfo/checkin-list



More information about the developers-list mailing list