| Bug #44834 | strxnmov is expected to behave as you'd expect | ||
|---|---|---|---|
| Submitted: | 12 May 2009 21:20 | Modified: | 3 Aug 2009 23:45 |
| Reporter: | Daniel Fischer | Email Updates: | |
| Status: | Closed | Impact on me: | |
| Category: | MySQL Server: Charsets | Severity: | S1 (Critical) |
| Version: | any | OS: | Any |
| Assigned to: | Sergei Glukhov | CPU Architecture: | Any |
[13 May 2009 0:05]
Davi Arnaut
In a nutshell: there will be a one byte overflow if the real size of the buffer is passed to strxnmov and this size is exceed by the length of the strings being concatenated.
Suggested fix:
=== modified file 'strings/strxnmov.c'
--- strings/strxnmov.c 2007-05-10 09:59:39 +0000
+++ strings/strxnmov.c 2009-05-13 00:05:05 +0000
@@ -42,7 +42,7 @@
char *strxnmov(char *dst, size_t len, const char *src, ...)
{
va_list pvar;
- char *end_of_dst=dst+len;
+ char *end_of_dst=dst+len-1;
va_start(pvar,src);
while (src != NullS)
[13 May 2009 5:48]
Daniel Fischer
Davi, it's not as easy, because in many places the code relies on strxnmov acting the way it does and just changing it without changing other parts of the server will lead to behavioural differences as limits are reduced by one.
[15 Jun 2009 13:51]
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/76282 2936 Sergey Glukhov 2009-06-15 Bug#44834 strxnmov is expected to behave as you'd expect The problem: described in the bug report. The fix: --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ client/mysql.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/ha_ndbcluster.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/ha_ndbcluster_binlog.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/handler.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/mysqld.cc removed unnecessary line @ sql/parse_file.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_acl.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_base.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_db.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_delete.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_partition.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_rename.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_show.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_table.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_view.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function
[17 Jun 2009 12:41]
Georgi Kodinov
I see few more instances of where we should change the code : sql/ha_ndbcluster.cc char* ha_ndbcluster::get_tablespace_name() 9502 strxnmov(name, name_len, ts.getName(), NullS); sql/log.cc bool LOGGER::slow_log_print() 945 char user_host_buff[MAX_USER_HOST_SIZE]; 969 /* fill in user_host value: the format is "%s[%s] @ %s [%s]" */ 970 user_host_len= (strxnmov(user_host_buff, MAX_USER_HOST_SIZE, 971 sctx->priv_user ? sctx->priv_user : "", "[", 972 sctx->user ? sctx->user : "", "] @ ", 973 sctx->host ? sctx->host : "", " [", 974 sctx->ip ? sctx->ip : "", "]", NullS) - 975 user_host_buff); bool LOGGER::general_log_write() 1011 char user_host_buff[MAX_USER_HOST_SIZE]; 1028 user_host_len= strxnmov(user_host_buff, MAX_USER_HOST_SIZE, 1029 sctx->priv_user ? sctx->priv_user : "", "[", 1030 sctx->user ? sctx->user : "", "] @ ", 1031 sctx->host ? sctx->host : "", " [", 1032 sctx->ip ? sctx->ip : "", "]", NullS)
[19 Jun 2009 8:33]
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/76641 2936 Sergey Glukhov 2009-06-19 Bug#44834 strxnmov is expected to behave as you'd expect The problem: described in the bug report. The fix: --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used @ client/mysql.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/ha_ndbcluster.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/ha_ndbcluster_binlog.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/handler.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/log.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/mysqld.cc removed unnecessary line @ sql/parse_file.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_acl.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_base.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_db.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_delete.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_partition.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_rename.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_show.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_table.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_view.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function
[19 Jun 2009 9:27]
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/76648 2961 Sergey Glukhov 2009-06-19 Bug#44834 strxnmov is expected to behave as you'd expect The problem: described in the bug report. The fix: --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used @ client/mysql.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/ha_ndbcluster.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/ha_ndbcluster_binlog.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/handler.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/log.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/mysqld.cc removed unnecessary line @ sql/parse_file.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_acl.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_base.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_db.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_delete.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_partition.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_rename.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_show.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_table.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function @ sql/sql_view.cc --increase buffers where it's necessary (buffers which are used in stxnmov) --decrease buffer lengths which are used as argument for strxnmov function
[8 Jul 2009 13:30]
Bugs System
Pushed into 5.1.37 (revid:joro@sun.com-20090708131116-kyz8iotbum8w9yic) (version source revid:sergey.glukhov@sun.com-20090619082443-1u6dm75s5gciyjso) (merge vers: 5.1.36) (pib:11)
[9 Jul 2009 7:36]
Bugs System
Pushed into 5.1.37 (revid:joro@sun.com-20090708131116-kyz8iotbum8w9yic) (version source revid:sergey.glukhov@sun.com-20090619082443-1u6dm75s5gciyjso) (merge vers: 5.1.36) (pib:11)
[10 Jul 2009 11:20]
Bugs System
Pushed into 5.4.4-alpha (revid:anozdrin@bk-internal.mysql.com-20090710111017-bnh2cau84ug1hvei) (version source revid:sergey.glukhov@sun.com-20090619092844-7xf8myliowe8i9iv) (merge vers: 5.4.4-alpha) (pib:11)
[3 Aug 2009 23:45]
Paul DuBois
Noted in 5.1.37, 5.4.4 changelogs. The strxnmov() library function could write a null byte after the end of the destination buffer.
[12 Aug 2009 21:48]
Paul DuBois
Noted in 5.4.2 changelog because next 5.4 version will be 5.4.2 and not 5.4.4.
[14 Aug 2009 22:46]
Paul DuBois
Ignore previous comment about 5.4.2.
[26 Aug 2009 13:45]
Bugs System
Pushed into 5.1.37-ndb-7.0.8 (revid:jonas@mysql.com-20090826132541-yablppc59e3yb54l) (version source revid:jonas@mysql.com-20090826132541-yablppc59e3yb54l) (merge vers: 5.1.37-ndb-7.0.8) (pib:11)
[26 Aug 2009 13:46]
Bugs System
Pushed into 5.1.37-ndb-6.3.27 (revid:jonas@mysql.com-20090826105955-bkj027t47gfbamnc) (version source revid:jonas@mysql.com-20090826105955-bkj027t47gfbamnc) (merge vers: 5.1.37-ndb-6.3.27) (pib:11)
[26 Aug 2009 13:48]
Bugs System
Pushed into 5.1.37-ndb-6.2.19 (revid:jonas@mysql.com-20090825194404-37rtosk049t9koc4) (version source revid:jonas@mysql.com-20090825194404-37rtosk049t9koc4) (merge vers: 5.1.37-ndb-6.2.19) (pib:11)
[27 Aug 2009 16:32]
Bugs System
Pushed into 5.1.35-ndb-7.1.0 (revid:magnus.blaudd@sun.com-20090827163030-6o3kk6r2oua159hr) (version source revid:jonas@mysql.com-20090826132541-yablppc59e3yb54l) (merge vers: 5.1.37-ndb-7.0.8) (pib:11)
[7 Oct 2009 1:25]
Paul DuBois
The 5.4 fix has been pushed into 5.4.2.
Description: There is a significant difference between strxnmov in MySQL 5.0 and strxnmov in MySQL 5.1 and above. In MySQL 5.0, strxnmov will only NUL-terminate a result string if the components concatenated are shorter than the buffer size: "moves the first len characters of the concatenation of src1,...,srcn to dst. If there aren't that many characters, a NUL character will be added to the end of dst to terminate it properly." In MySQL 5.1 and above, strxnmov will always NUL-terminate a result string. If the input strings are longer than the buffer, the terminating NUL byte will be written to the byte *directly after the end of the buffer* as specified by the buffer size passed into strxnmov: "moves the first len characters of the concatenation of src1,...,srcn to dst and add a closing NUL character." This causes two different problems in MySQL 5.0 and below, and in MySQL 5.1 and above. In MySQL 5.0 and below, we sometimes make the assumption that strxnmov NUL-terminates its result when in reality it does not. In some of these cases, we later call strlen() or similar functions that expect a NUL-terminated string on the result of strxnmov. This causes a buffer overread. In some cases (such as the "help" command, see "how to repeat" for example), data after the end of the buffer is sent to a network peer. In MySQL 5.1 and above, we sometimes make the assumption that strxnmov will never write beyond the end of the buffer. In many places, we substract one from the buffer length passed into strxnmov. However, in some places, this adjustment is not made. This results in a buffer overrun, albeit a fairly tame one, since you only get to write one NUL byte to the stack. Unfortunately, this was not detected when bug#43153 was fixed. The code that was replaced in bug#43153 was one of the places where this buffer overrun occurred. It was visible as described in bug#44804. The fix for bug#43153 eliminates this *single specific* buffer overrun and therefore makes bug#44804 invisible again. Other places where strxnmov is used badly include the following. The list is non-exhaustive, but most of these mentions apply to both MySQL 5.0 and below and MySQL 5.1 and above, since the assumption that strxnmov acts like strncpy is wrong in both cases: mysql.cc:com_server_help sql_table.cc ha_ndbcluster.cc handler.cc log.cc How to repeat: See bug#44804 for a way to repeat this quickly with older sources (before the bug#43153 fix). This type of bug often doesn't have measurable effects, but to repeat this bug in MySQL 5.1 and above, one could add a sentinel byte after the end of a buffer that is used with strxnmov and verify that it is not modified by strxnmov. I'm including a patch that adds a sentinel byte to the buffer used to piece together a help command before it is sent to the server. The bug is not detectable in the original code because the NUL byte is (pretty much) always written to stack space used for other variables local to the function that are only used later on. The weird union/struct construct is there to circumvent padding on some platforms. --- client/mysql.cc.org 2009-05-12 22:43:11.000000000 +0200 +++ client/mysql.cc 2009-05-12 22:43:24.000000000 +0200 @@ -2757,9 +2757,16 @@ { MYSQL_ROW cur; const char *server_cmd= buffer->ptr(); + union { char cmd_buf[100]; + struct { + char foo[100]; + char bar; + } x; + }; MYSQL_RES *result; int error; + x.bar = 99; if (help_arg[0] != '\'') { char *end_arg= strend(help_arg); @@ -2771,6 +2778,7 @@ } (void) strxnmov(cmd_buf, sizeof(cmd_buf), "help '", help_arg, "'", NullS); server_cmd= cmd_buf; + assert(x.bar == 99); } if (!status.batch) After this patch is applied, a help command at least this long will trigger the assertion: help xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx The same help command can be used on an unpatched MySQL 5.0 to trigger a buffer overread. An error 1064 is expected, since we quote the help topic before sending the command to the server, and when the topic is too long, the terminating single quote will not fit into the buffer. Under some conditions, the buffer overread will result in garbage output in the error message such as the following: ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '�' at line 1 Suggested fix: It would be easy to suggest that, since strxnmov is so often used in apparent ignorance of its uniquely strance interface, it should be changed to always NUL-terminate its output within the result buffer, and never write beyond the end of the result buffer. However, this will reduce its utility in conditions where NUL-terminated strings are not necessary. Possibly, strxnmov should be split into two separate functions, one that NUL-terminates its input, and one that doesn't. At any rate, the current situation is highly dangerous as this kind of bug, which is a very likely mistake to make, will be invisible in many cases; either due to padding (extra bytes around the buffer that will provide the one byte needed for the extra NUL) or due to local variables around the buffer that can be used to store the extra NUL with no ill results.