MySQL Bugs: #26233: very suspect code in mf_tempfile.c, in function create_temp

Bug #26233	very suspect code in mf_tempfile.c, in function create_temp_file()
Submitted:	9 Feb 2007 17:35	Modified:	10 Apr 2007 18:07
Reporter:	Shane Bester (Platinum Quality Contributor)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: General	Severity:	S2 (Serious)
Version:	5.0,5.1	OS:	Windows (windows)
Assigned to:	Magnus Blåudd	CPU Architecture:	Any
Tags:	bfsm_2007_02_15

Description:
Reviewing the code in function create_temp_file() in the file mf_tempfile.c reveals some bad programming.

Especially bad is the switching of the mysqld's global environ variable. 

environ=temp_env;		/* Force use of dir (dir not checked) */
temp_env[0]=0;

The above doesn't look very thread safe to me.

How to repeat:
read the code.

Suggested fix:
cleanup the code. especially keep in mind how the code runs in a highly concurrent situation.  be sure no memory corruption happen, even for a split second.

Shane Bester wrote:
> Hi!
>
>
> In mf_tempfile.c we have the following code. Is it safe to play around
> with the "environ" variable
> like this code does in highly concurrent environments? seen multiple
> crashes inside tempnam() which
> calls getenv("TMP") is the reason why I ask.

I would say the whole function is kind of obscure, badly documented and
ifdefs all over.

It looks like "environ" is modified to point at an empty environ on the
local stack. Since environ is a global variable for the whole binary
all other threads would see the modified environ. Especially since
"temp_env[0]" is set to 0 on the line _after_ it's assigned to environ
we have a small moment where environ is pointing at undefined memory.

It's hard to find out exactly from where this function is used, but it
for sure is used by ha_innodb.cc which definitely is in mysqld.

Did you see the crash in mysqld or in a client using libmysqlclient?

I would suggest filing a bug requesting the function to be
documented(after figuring out what it should do) and rewritten, there
must be a more generic way to do this.

Best regards
Magnus

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/22742

ChangeSet@1.2466, 2007-03-23 11:01:47+01:00, msvensson@pilot.blaudden +3 -0
  Bug#26233 very suspect code in mf_tempfile.c, in function create_temp_file()
   - Rework the windows implementation in 'create_temp_file' to be
     thread safe by using GetTempFileName instad of fiddling
     with "environ"

Pushed into 5.0.40

Pushed into 5.1.18-beta

Noted in 5.0.40, 5.1.18 changelogs.

The temporary file-creation code was cleaned up on Windows to improve
server stability.