| Bug #48983 | Bad strmake calls (length one too long) | ||
|---|---|---|---|
| Submitted: | 23 Nov 2009 10:41 | Modified: | 12 Mar 2010 16:47 |
| Reporter: | Mattias Jonsson | Email Updates: | |
| Status: | Closed | Impact on me: | |
| Category: | MySQL Server: General | Severity: | S2 (Serious) |
| Version: | 5.0, 5.1, 5.6 bzr | OS: | Any |
| Assigned to: | Davi Arnaut | CPU Architecture: | Any |
[23 Nov 2009 10:41]
Mattias Jonsson
[23 Nov 2009 10:43]
Mattias Jonsson
Here is some notes from my own findings of bad strmake calls:
libmysql/libmysql.c mysql_change_user uses a too small buffer
char buff[USERNAME_LENGTH+SCRAMBLED_PASSWORD_CHAR_LENGTH+NAME_LEN+2];
char *end= buff;
end= strmake(end, user, USERNAME_LENGTH) + 1;
/* now USERNAME_LENGTH + 1 */
if (passwd[0])
{
if (mysql->server_capabilities & CLIENT_SECURE_CONNECTION)
{
*end++= SCRAMBLE_LENGTH;
scramble(end, mysql->scramble, passwd);
end+= SCRAMBLE_LENGTH;
/* now USERNAME_LENGTH + 1 + 1 + SCRAMBLE_LENGTH (20) =>
USERNAME_LENGTH + 1 + 1 + SHA1_HASH_SIZE (20)
OK, always at least 19 bytes too long instead of too short...
*/
end= strmake(end, db ? db : "", NAME_LEN) + 1;
if (mysql->server_capabilities & CLIENT_SECURE_CONNECTION)
{
int2store(end, (ushort) mysql->charset->number);
end+= 2;
}
max is now USERNAME_LENGTH + 1 + 1 + SCRAMBLE_LENGTH + NAME_LEN + 1 + 2
read_user_name:
strmake(name, str ? str : "UNKNOWN_USER", USERNAME_LENGTH);
mysql_real_connect uses too small buffer:
char name_buff[USERNAME_LENGTH];
...
read_user_name(name_buff);
Also in sql-common/client.c (but may be OK?!?!)
if (user && user[0])
strmake(end,user,USERNAME_LENGTH); /* Max user name */
else
read_user_name((char*) end);
Maybe OK, since it uses 'end' after this too...
mysql_list_fields(MYSQL *mysql, const char *table, const char *wild)
{
MYSQL_RES *result;
MYSQL_FIELD *fields;
char buff[257],*end;
DBUG_ENTER("mysql_list_fields");
DBUG_PRINT("enter",("table: '%s' wild: '%s'",table,wild ? wild : ""));
end=strmake(strmake(buff, table,128)+1,wild ? wild : "",128);
If table and wild is >= 128 then buf[258] (*end) ? will be overwritten!
in server-tools/instance-manager/instance_map.cc:
static void parse_option(const char *option_str,
char *option_name_buf,
char *option_value_buf)
{
const char *eq_pos;
const char *ptr= option_str;
while (*ptr == '-')
++ptr;
strmake(option_name_buf, ptr, MAX_OPTION_LEN + 1);
in server-tools/instance-manager/listener.cc create_unix_socket:
strmake(unix_socket_address.sun_path, Options::Main::socket_file_name,
sizeof(unix_socket_address.sun_path));
in server-tools/instance-manager/options.cc setup_windows_defaults:
char base_name[FN_REFLEN];
char im_name[FN_REFLEN];
strmake(base_name, base_name_ptr, FN_REFLEN);
*base_name_ptr= 0;
strmake(im_name, base_name, FN_REFLEN);
in server-tools/instance-manager/user_map.cc User::User:
user_length= (uint8) (strmake(user, user_name_arg->str,
USERNAME_LENGTH + 1) - user);
from user_map.h:
char user[USERNAME_LENGTH + 1];
in sql/log.cc MYSQL_LOG::generate_name:
strmake(buff, log_name, min(length, FN_REFLEN));
(open_slow_log:
char buf[FN_REFLEN];
return open(generate_name(log_name, ".log", 0, buf), LOG_NORMAL, 0,
WRITE_CACHE);
)
And the same for open-query_log...
in sql/log.cc MYSQL_BIN_LOG::purge_logs_before_date:
char to_log[FN_REFLEN];
strmake(to_log,
log_info.log_file_name,
sizeof(log_info.log_file_name));
typedef struct st_log_info
{
char log_file_name[FN_REFLEN];
Just looking at get_object in sql/repl_failsafe.cc i got scared :) (ok saves some lines, but no beauty at all) but correct.
in sql/rpl_mi.cc init_master_info_with_options:
if (master_password)
strmake(mi->password, master_password, MAX_PASSWORD_LENGTH);
in sql/sql_acl.cc acl_getroot and acl_getroot_no_password:
if (acl_user->host.hostname)
strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME);
Hmm STRING_BUFFER_USUAL_SIZE, lack of imagination when brainstorming for name?
in sql/sql_plugin.cc check_func_set:
strmake(buff, error, min(sizeof(buff), error_len));
in sql/sql_table.cc mysql_prepare_create_table:
char tmp[64];
strmake(strmake(tmp, save_cs->csname, sizeof(tmp)-4),
STRING_WITH_LEN("_bin"));
in sql-common/client.c...
in strings/ctype-tis620.c my_strnxfrm_tis620:
len= (size_t) (strmake((char*) dest, (char*) src, min(len, srclen)) -
(char*) dest);
I have not looked at the assembler code for strmake...
[25 Nov 2009 22:57]
Sveta Smirnova
Thank you for the report.
Verified as described.
To repeat:
add to strmake call output of n to error log: fprintf(stderr, "N: %d\n", n);
$bzr diff
=== modified file 'strings/strmake.c'
--- strings/strmake.c 2009-07-16 13:17:47 +0000
+++ strings/strmake.c 2009-11-25 22:49:10 +0000
@@ -50,5 +50,6 @@
if (! (*dst++ = *src++))
return dst-1;
*dst=0;
+ fprintf(stderr, "N: %d\n", n);
return dst;
}
Run test partition_innodb_semi_consistent , save file var/log/mysqld.1.err
Modify strmake so it revert the patch for bug #46042:
=== modified file 'strings/strmake.c'
--- strings/strmake.c 2009-07-16 13:17:47 +0000
+++ strings/strmake.c 2009-11-25 22:38:09 +0000
@@ -41,14 +41,20 @@
write a character rather than '\0' as this makes spotting these
problems in the results easier.
*/
+ /*
uint n= 0;
while (n < length && src[n++]);
memset(dst + n, (int) 'Z', length - n + 1);
+ */
+ uint n= strlen(src) + 1;
+ if (n <= length)
+ memset(dst + n, (int) 'Z', length - n + 1);
#endif
while (length--)
if (! (*dst++ = *src++))
return dst-1;
*dst=0;
+ fprintf(stderr, "N: %d\n", n);
return dst;
}
Run test partition_innodb_semi_consistent, then compare saved mysqld.1.err and new one. You will see values of n would be different. Then compare if all strmake calls changed after fix for bug #46042 was applied. They are not.
[17 Dec 2009 17:59]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/94822 2824 Davi Arnaut 2009-12-17 Bug#48983: Bad strmake calls (length one too long) The problem is a somewhat common misusage of the strmake function. The strmake(dst, src, len) function writes at most /len/ bytes to the string pointed to by src, not including the trailing null byte. Hence, if /len/ is the exact length of the destination buffer, a one byte buffer overflow can occur if the length of the source string is equal to or greater than /len/. @ client/mysqldump.c Make room for the trailing null byte. @ libmysql/libmysql.c Add comment, there is enough room in the buffer. Increase buffer length, two strings are concatenated. @ libmysqld/lib_sql.cc Make room for the trailing null byte. @ mysys/default.c Make room for the trailing null bytes. @ mysys/mf_pack.c Make room for the trailing null byte. @ server-tools/instance-manager/commands.cc Copy only if overflow isn't possible in both cases. @ server-tools/instance-manager/listener.cc Make room for the trailing null byte. @ sql/log.cc Make room for the trailing null byte. @ sql/sp_pcontext.h Cosmetic fix. @ sql/sql_acl.cc MAX_HOSTNAME already specifies space for the trailing null byte. @ sql/sql_parse.cc Make room for the trailing null byte. @ sql/sql_table.cc Make room for the trailing null byte.
[18 Dec 2009 19:14]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/94997 3286 Davi Arnaut 2009-12-18 Bug#48983: Bad strmake calls (length one too long) MySQL 5.1 specific fixes.
[18 Dec 2009 19:16]
Davi Arnaut
Queued to mysql-5.0-bugteam and up.
[14 Jan 2010 8:26]
Bugs System
Pushed into 5.0.90 (revid:joro@sun.com-20100114082402-05fod2h6z9x9wok8) (version source revid:davi.arnaut@sun.com-20091217175838-pzt20wyp63immpzl) (merge vers: 5.0.89) (pib:16)
[15 Jan 2010 9:01]
Bugs System
Pushed into 5.1.43 (revid:joro@sun.com-20100115085139-qkh0i0fpohd9u9p5) (version source revid:davi.arnaut@sun.com-20091218191409-gdgq8s7ctl6twd01) (merge vers: 5.1.42) (pib:16)
[22 Jan 2010 1:24]
Paul DuBois
Noted in 5.0.90, 5.1.43 changelogs. Several strmake() calls had an incorrect length argument (too large by one). Setting report to NDI pending push to 5.5.x+.
[5 Feb 2010 11:48]
Bugs System
Pushed into mysql-next-mr (revid:alik@sun.com-20100204063540-9czpdmpixi3iw2yb) (version source revid:alik@sun.com-20091224075613-es9uswo4lidkm3tj) (pib:16)
[5 Feb 2010 11:55]
Bugs System
Pushed into 6.0.14-alpha (revid:alik@sun.com-20100205113942-oqovjy0eoqbarn7i) (version source revid:alik@sun.com-20100204064210-ljwanqvrjs83s1gq) (merge vers: 6.0.14-alpha) (pib:16)
[5 Feb 2010 12:01]
Bugs System
Pushed into 5.5.2-m2 (revid:alik@sun.com-20100203172258-1n5dsotny40yufxw) (version source revid:alexey.kopytov@sun.com-20091223134205-pk9yvgfvpn3hy7lh) (merge vers: 5.5.1-m2) (pib:16)
[5 Feb 2010 17:11]
Paul DuBois
Noted in 5.5.2, 6.0.14 changelogs. Setting report to Need Merge pending push to Celosia.
[12 Mar 2010 14:15]
Bugs System
Pushed into 5.1.44-ndb-7.0.14 (revid:jonas@mysql.com-20100312135944-t0z8s1da2orvl66x) (version source revid:jonas@mysql.com-20100312115609-woou0te4a6s4ae9y) (merge vers: 5.1.44-ndb-7.0.14) (pib:16)
[12 Mar 2010 14:31]
Bugs System
Pushed into 5.1.44-ndb-6.2.19 (revid:jonas@mysql.com-20100312134846-tuqhd9w3tv4xgl3d) (version source revid:jonas@mysql.com-20100312060623-mx6407w2vx76h3by) (merge vers: 5.1.44-ndb-6.2.19) (pib:16)
[12 Mar 2010 14:47]
Bugs System
Pushed into 5.1.44-ndb-6.3.33 (revid:jonas@mysql.com-20100312135724-xcw8vw2lu3mijrhn) (version source revid:jonas@mysql.com-20100312103652-snkltsd197l7q2yg) (merge vers: 5.1.44-ndb-6.3.33) (pib:16)
[12 Mar 2010 16:47]
Paul DuBois
Fixed in earlier 5.1.x, 5.5.x.
