MySQL Bugs: #31588: buffer overrun when setting variables

Bug #31588	buffer overrun when setting variables
Submitted:	14 Oct 2007 15:31	Modified:	30 Oct 2007 1:00
Reporter:	Shane Bester (Platinum Quality Contributor)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: General	Severity:	S1 (Critical)
Version:	5.0.50, 5.1.23	OS:	Any
Assigned to:	Tatiana Azundris Nuernberg	CPU Architecture:	Any
Tags:	buffer overrun

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.

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.

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. : )

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.

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.

- pushed to 4.1.24, 5.0.52, 5.1.23 in opt
- Code review suggestion added as Bug#31752

Pushed into 4.1.24

Pushed into 5.0.52

Pushed into 5.1.23-beta

Pushed into 6.0.4-alpha

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.