| Bug #31588 | buffer overrun when setting variables | ||
|---|---|---|---|
| Submitted: | 14 Oct 2007 17:31 | Modified: | 30 Oct 2007 2:00 |
| Reporter: | Shane Bester | ||
| Status: | Closed | ||
| Category: | Server: General | Severity: | S1 (Critical) |
| Version: | 5.0.50, 5.1.23 | OS: | Any |
| Assigned to: | Tatjana A. Nuernberg | Target Version: | |
| Tags: | buffer overrun | ||
[16 Oct 2007 15:31]
Tatjana A. Nuernberg
Analysis is indeed cqt. strmake(dst, src, len) copies over up to len characters, then appends '\0' if needed, so sizeof(dst) must be len + 1, or, conversely, len = sizeof(dst) -1. str incidently gets alloc'd in check_set() as its buffer is exactly full (STRING_BUFFER_USUAL_SIZE characters 'a') when we call c_ptr(), forcing a re-alloc for that size + 1 (to accommodate a terminating '\0'), plus alignment. To make things a little more interestring, if (!(res= var->value->val_str(&str))) uses str for value-length up to and including STRING_BUFFER_USUAL_SIZE (payload, not counting '\0'), so for < S/B/U/S, everything is correct, for > S/B/U/S res has its own buffer (We still overrun 'buff', but we call c_ptr() on res, not str, so str never gets realloc'd, and consequently, we don't try to free it when it goes out of scope. The overrunning of 'buff' still ends up destroying the pointer of str, but since we do not try to dealloc the broken address, we get a lucky escape thanks to the stack-layout.). It's only for == S/B/U/S that we get a re-alloc for str, an overrun of buff that destroys str's pointer, and a subsequent complaint when we try to free that pointer when str goes out of scope. Ah, what fun.
[16 Oct 2007 15:47]
Tatjana A. Nuernberg
On a side-note, we can safely
strmake(buff, error, min(sizeof(buff) - 1, error_len));
because a) we already limited the size before (if off by one), so the behaviour, at least
in spirit, does not change; and b) for current settings, that makes the buffer 79+1
characters, while the error-message limits the string to 64 characters anyway. : )
[17 Oct 2007 13:28]
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/35746 ChangeSet@1.2536, 2007-10-17 13:29:18+02:00, tnurnberg@sin.intern.azundris.com +3 -0 Bug#31588: buffer overrun when setting variables Buffer used when setting variables was not dimensioned to accomodate trailing '\0'. An overflow by one character was therefore possible. CS corrects limits to prevent such overflows. The actual error message using this buffered value only prints the first 64 characters of it anyway, so this causes no change visible to the user at this point.
[18 Oct 2007 10:46]
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/35816 ChangeSet@1.2686, 2007-10-18 10:47:54+02:00, tnurnberg@sin.intern.azundris.com +3 -0 Bug#31588: buffer overrun when setting variables Buffer used when setting variables was not dimensioned to accomodate trailing '\0'. An overflow by one character was therefore possible. CS corrects limits to prevent such overflows.
[22 Oct 2007 15:42]
Tatjana A. Nuernberg
- pushed to 4.1.24, 5.0.52, 5.1.23 in opt - Code review suggestion added as Bug#31752
[29 Oct 2007 9:40]
Bugs System
Pushed into 4.1.24
[29 Oct 2007 9:43]
Bugs System
Pushed into 5.0.52
[29 Oct 2007 9:47]
Bugs System
Pushed into 5.1.23-beta
[29 Oct 2007 9:51]
Bugs System
Pushed into 6.0.4-alpha
[30 Oct 2007 2:00]
Paul DuBois
Noted in 4.1.24, 5.0.52, 5.1.23, 6.0.4 changelogs. A buffer used when setting variables was not dimensioned to accommodate the trailing '\0' byte, so a single-byte buffer overrun was possible.
Description: There's a 1 byte buffer overrun when setting variable to a string value equal to or longer than STRING_BUFFER_USUAL_SIZE (80) characters. E:\mysql-5.1.22-rc-win32\bin>mysqld-debug --console --skip-grant-tables 071014 17:00:30 InnoDB: Started; log sequence number 0 1980191712 071014 17:00:30 [Note] mysqld-debug: ready for connections. Version: '5.1.22-rc-community-debug' socket: '' port: 3306 MySQL Community Server - Debug (GPL) Error:Run-Time Check Failure #2 - Stack around the variable 'buff' was corrupted. At :0 Error:Run-Time Check Failure #2 - Stack around the variable 'buff' was corrupted. At :0 Error:Run-Time Check Failure #2 - Stack around the variable 'buff' was corrupted. At :0 Error:Run-Time Check Failure #2 - Stack around the variable 'buff' was corrupted. At :0 How to repeat: set global sql_mode=repeat('a',80); Needless to say, a 1 byte overrun is not always going to crash. It crashes debug binaries on windows, and the release binary does assert. Study the code or run under valgrind will be easy to check. Suggested fix: Fix the function bool sys_var::check_set(THD *thd, set_var *var, TYPELIB *enum_names) There is this code: if (error_len) { strmake(buff, error, min(sizeof(buff), error_len)); goto err; } I suspect it should be sizeof(buff)-1 to give room for null terminator.