| Bug #43918 | Generated version string is too long for the binlog | ||
|---|---|---|---|
| Submitted: | 27 Mar 2009 17:45 | Modified: | 4 May 2009 9:05 |
| Reporter: | Joerg Bruehe | Email Updates: | |
| Status: | Verified | Impact on me: | |
| Category: | MySQL Server: Security: Privileges | Severity: | S3 (Non-critical) |
| Version: | 5.1.31sp1 | OS: | Any |
| Assigned to: | Kent Boortz | CPU Architecture: | Any |
[27 Mar 2009 17:47]
Joerg Bruehe
Defect assessment is also based on embarrassment: Our own binaries should pass our own tests.
[1 May 2009 23:00]
Bugs System
No feedback was provided for this bug for over a month, so it is being suspended automatically. If you are able to provide the information that was originally requested, please do so and change the status of the bug back to "Open".
[4 May 2009 9:05]
Joerg Bruehe
Seems I forgot to maintain the status :(
[6 Aug 2009 17:26]
Joerg Bruehe
Some remarks about the components of the version comment:
5.1.31sp1-enterprise-commercial-advanced-debug-log
5.1.31sp1 = obviously, the version number.
Note that with a custom build it would be even longer, like
5.1.31-br12345
so we have this bug in many custom builds, depending on the configuration name.
The value comes from "configure.in" and is passed via a CPP symbol:
include/mysql_version.h.in:#define MYSQL_SERVER_VERSION "@VERSION@"
-enterprise-commercial-advanced = a string identifying the configuration
Its origin is the "--with-server-suffix" argument to "configure" which is truncated to 35 characters (which it never meets, this one is 31).
Again, it is passed via a CPP symbol:
include/mysql_version.h.in:#define MYSQL_SERVER_SUFFIX_DEF "@MYSQL_SERVER_SUFFIX@"
If it is not overridden by a "-D" argument, it is passed onwards here:
sql/mysqld_suffix.h:#define MYSQL_SERVER_SUFFIX_STR MYSQL_SERVER_SUFFIX_DEF
-debug = Appended by server code for debug builds
-log = Appended by server code if some logging option is given
The final string is combined in "set_server_version()", sql/mysqld.cc
The version part length is at least 6 characters ("5.1.34"),
9 characters for a QSP ("5.1.34sp1"),
or up to 14 characters for a custom build ("5.1.34-br34567").
The longest suffix I am aware of is 31 characters long:
"-enterprise-commercial-advanced"
Any server in real use will append 4 characters ("-log").
Here is a list of values from 5.1.34sp1:
(50) 5.1.34sp1-enterprise-commercial-advanced-debug-log
(44) 5.1.34sp1-enterprise-commercial-advanced-log
(49) 5.1.34sp1-enterprise-commercial-classic-debug-log
(43) 5.1.34sp1-enterprise-commercial-classic-log
(45) 5.1.34sp1-enterprise-commercial-pro-debug-log
(39) 5.1.34sp1-enterprise-commercial-pro-log
(43) 5.1.34sp1-enterprise-gpl-advanced-debug-log
(37) 5.1.34sp1-enterprise-gpl-advanced-log
(38) 5.1.34sp1-enterprise-gpl-pro-debug-log
(32) 5.1.34sp1-enterprise-gpl-pro-log
For a custom build, the strings are 5 characters longer:
"sp1" -> "-br34567"
For community builds, all this is no trouble:
5.1.34-debug-log
5.1.34-log
To fix this bug, I see these different approaches:
1) Expand all variables and fields (log format!) taking the string,
we need at least 60 characters.
pro: This is a really clean solution.
con: I don't know whether this causes compatibility problems,
especially in mixed version replications etc.
2) Abbreviate the string "commercial", to "comm" or "com".
pro: No format change.
con: Gain is limited to 6 or 7 characters,
and it needs quite tricky special handling in packaging tools
because the suffix also becomes part of the file names
(and maybe we don't want to change these).
3) Drop "-enterprise" from the version string in the server.
pro: No format change,
and we gain 11 characters,
and we can leave the string constant externally,
just drop a leading "-enterprise" when it enters the server.
con: Different string in the file names and in the version output.
IMO,
1) is really clean, but I have no guess about effort and risk,
2) is no good choice, as the gain is low and the whole approach is a hack,
3) is sufficiently easy to implement and fairly safe.
[7 Aug 2009 10:16]
Joerg Bruehe
My last remark is incomplete:
We use the version string in (at least) three ways:
- within the server, to identify,
- as part of the package file name,
- in the test client and test streams, to recognize it and replace by a constant
(without this, test results would differ by version).
So we can't simply drop "-enterprise" in the server only,
we would need to do the same in the test client (so that replacement still works).
This means, I have to replace solution 3 (my last comment) by
3a) drop the "-enterprise" completely, from string and package file names.
pro: clean, easy, very simple and barely any risk,
we reduce our overly long file and directory names
(tar problems like in bug#38453 and bug#41932).
con: changes package file names,
thus affecting customer habits and publishing tools.
3b) drop "-enterprise" from the string output by the server,
but keep it in the package file names.
pro: easy, only little risk, no change in file names.
con: need to differ between use in
string (drop, in both the server and the test client)
versus file name (keep).
So I still
- prefer solution 1 (extremely clean), but see the risks and the effort,
- consider solution 2 quite hacky and don't really like it,
- think 3a is equally good as 1,
if acceptable to the customers and to publishing,
and has the advantage of reducing name lengths,
- think 3b is technically acceptable as an immediate fix,
but should be replaced by 3a for a new GA version.
[11 Dec 2009 17:56]
Kent Boortz
I think the conclusion after discussing this is that the length of the server
suffix is not really a problem. It will be truncated and is really only
informative in the binlogs. So this is a test case that needs to be improved.
But I anyway promised to include some findings when looking into this.
There "might" be risks of buffer overflows, but don't take the claims below
as the "truth", it is just from reading the source, and was written before
the conclusion the binlogs are fine.
There is a global in "sql/mysqld.cc"
char server_version[SERVER_VERSION_LENGTH];
that has the max length set in "include/mysql_com.h"
#define SERVER_VERSION_LENGTH 60
but the one for use in the binlog format is even shorter, defined in
"log_event.h" and deliberately decoupled from the
SERVER_VERSION_LENGTH because the binlog one can't be changed
#define ST_SERVER_VER_LEN 50
The 'server_version' is intially set to the long version string, i.e.
"5.1.40sp1" or "5.5.87-alpha", but just a few micro seconds later set
to a much longer string
long-server-version + mysql-server-suffix + [ "-embedded" ] + [ "-debug" ] + [ "-log" ]
So, lets try a worst case scenario where we leave out the server suffix
5.1.40sp1-br12345-embedded-debug-log
This is 36 characters, lets say 40 is the worst case. This leaves 10
(if going for the 50 char limit) characters for the server suffix. I
think the longest one is
-enterprise-commercial-advanced
This is 31, we have a buffer overflow... :( [Note, or not, not sure....]
There is one more complication, the code that handle the creation of
'server_version' really needs a cleanup. There are no tests for
writing outside of the buffer, and this might even mess up the
binlogs [Note, likely not a problem in the binlog code]. This is from
"sql/mysqld.cc"
static void set_server_version(void)
{
char *end= strxmov(server_version, MYSQL_SERVER_VERSION,
MYSQL_SERVER_SUFFIX_STR, NullS);
#ifdef EMBEDDED_LIBRARY
end= strmov(end, "-embedded");
#endif
#ifndef DBUG_OFF
if (!strstr(MYSQL_SERVER_SUFFIX_STR, "-debug"))
end= strmov(end, "-debug");
#endif
if (opt_log || opt_update_log || opt_slow_log || opt_bin_log)
strmov(end, "-log"); // This may slow down system
}
Lets say when running the server we manage not to core but the buffer
is too small. This means the string inside 'server_version' has no
ending '\0' inside the buffer. If we then look at "sql/sql_connect.cc"
end= strnmov(buff, server_version, SERVER_VERSION_LENGTH) + 1;
then this line will not terminate its copy on '\0', it will terminate
the copy on the length limit, and *not* add a '\0'. I assume this will
mess up "something", not sure what. It is anyway not what was intended
I think. The non standard strnmov() is documented to behave like
strnmov(dst,src,length) moves length characters, or until end, of src to
dst and appends a closing NUL to dst if src is shorter than length.
The result is a pointer to the first NUL in dst, or is dst+n if dst was
truncated.
Looking at "sql/log_event.cc" the 'server_version' global and the
class/struct one are used intermixed, with no or little buffer overrun
checks. Unless I misunderstand the code, one strmov() assuming it has
an ending '\0', but likely more problems related to 'server_version'.
Description: Test failure in the build of 5.1.31sp1: ===== binlog.binlog_stm_blackhole 'mix' [ fail ] --- /PATH/mysql-test/suite/binlog/r/binlog_stm_blackhole.result +++ /PATH/mysql-test/suite/binlog/r/binlog_stm_blackhole.reject @@ -106,7 +106,7 @@ a show binlog events; Log_name Pos Event_type Server_id End_log_pos Info -master-bin.000001 # Format_desc # # Server ver: VERSION, Binlog ver: 4 +master-bin.000001 # Format_desc # # Server ver: 5.1.31sp1-enterprise-commercial-advanced-debug-lo, Binlog ver: 4 master-bin.000001 # Query # # use `test`; drop table t1,t2 master-bin.000001 # Query # # use `test`; create table t1 (a int) engine=blackhole master-bin.000001 # Query # # use `test`; BEGIN @@ -165,7 +165,7 @@ set autocommit=1; show binlog events; Log_name Pos Event_type Server_id End_log_pos Info -master-bin.000001 # Format_desc # # Server ver: VERSION, Binlog ver: 4 +master-bin.000001 # Format_desc # # Server ver: 5.1.31sp1-enterprise-commercial-advanced-debug-lo, Binlog ver: 4 master-bin.000001 # Query # # use `test`; create table t1 (a int) engine=blackhole master-bin.000001 # Query # # use `test`; BEGIN master-bin.000001 # Query # # use `test`; insert into t1 values(1) mysqltest: Result content mismatch ===== The test failure occurs because the version string is not replaced by the constant "VERSION", as the test file attempts to do: This, in turn, is caused by the version string truncation - the final should be "log", as can be seen in the build log when features are reported: ===== version 5.1.31sp1-enterprise-commercial-advanced-debug-log ===== In the run, the end is truncated from "log" to "lo". Yes, it is too long: ===== > echo 5.1.31sp1-enterprise-commercial-advanced-debug-lo | wc 1 1 50 ===== How to repeat: Run the tests on an "advanced" build done by the current tools, using a long version number (QSP build). Plain 5.1.31 used a similar expansion but passed, because it doesn't have the "sp1" part. Suggested fix: Revert the tool change that causes these long names. Besides breaking the tests, they are simply unpractical.