Re: MonetDB: Jul2015 - Try not to overflow snprintf buffers.

On Jul 24, 2015, at 16:36, Sjoerd Mullender 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@monetdb.org https://www.monetdb.org/mailman/listinfo/checkin-list