291724 - Bug in ntmisc.c: _tzname

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Thomas Spear]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

Please see emails linked by URL listed above.

It's overwriting a buffer it doesn't own and whose size is not
documented, and it should be writing only four characters but is writing
more.

Reproducible: Always

Steps to Reproduce:
1. Download a cvs version of wine prior to 20050422, and a win32 copy of Mozilla
or Firefox.
2. Install wine on linux and ensure it is running properly
3. Run the installer in wine.
Actual Results:  
Installer crashes

Expected Results:  
Installer should have installed program.

See emails above

Comment 1

[Bernard Alleysson]

Please copy the relevant information from the URL here (I couldn't find it
through all these mails), and ntmisc.c is NSPR

Comment 2

[Thomas Spear]

Basically, a user was encountering a crash trying to install win32 Mozilla (and
Firefox for that matter) under wine (for testing).

As quoted from Juan Lang:

I went over to lxr.mozilla.org/seamonkey and searched for
GetTimeZoneInformation.  That produced the following file:
 http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/md/windows/ntmisc.c

They're doing a WideCharToMultiByte into the global variable _tzname.  As
the comment says, perhaps they shouldn't be..  Anyway, _tzname should live
in msvcrt, but our include/msvcrt/time.h has:
/* FIXME: Must do something for _daylight, _dstbias, _timezone, _tzname */

It seems that the maximum size for the TZ environment variable is 15 chars (16
with the NULL) as referenced here:
http://msdn.microsoft.com/library/en-us/vclib/html/_crt__tzset.asp

The corresponding names in _tzname are probably only 3 chars in length, so
4 bytes long including the NULL terminator.

Alexandre Julliard, the project admin, replied with: Actually, judging from the
pointer values I get on my XP box the buffers seem to be 64 chars long (though I
agree they will probably always contain at most 3 chars).

Finally, Juan Lang replied with: The comments explain the motivation: on some
versions of Windows NT, the value is incorrectly set for some time zones. 
Nonetheless the code is doing the wrong thing: it's converting an entire
string, e.g. "GMT Standard Time", rather than the time zone portion alone
("GMT").

-----

We worked up a patch for wine to conform to windows, so that the installers dont
crash, but as they said, it is trying to convert the whole string "GMT Standard
Time", when _tzname should be (as the name implies) only converting the name of
the time zone "GMT".  I should add that I am just a maintainer of the bugzilla
db over there and have nothing to do with the high level language coding, but as
one of the bugzilla maintainers I went ahead and got a go ahead to report it to
you guys.  If you have any more questions or need any further info, feel free to
ask and I will dig up what I can.

Comment 3

[Bernard Alleysson]

* msvcrt has the same code (_tzset(), file tzset.c)
* The size of msvcrt static _tzname[] buffers are 64 bytes
(excerpt offile timeset.c:
/* note that NT Posix's TZNAME_MAX is only 10 */
static char tzstd[64] = { "PST" };
static char tzdst[64] = { "PDT" };
char *_tzname[2] = { tzstd, tzdst };
)
* Note that limits.h has
#define TZNAME_MAX 10 and #define _POSIX_TZNAME_MAX 3

This suggests that NSPR should use TZNAME_MAX (so the size is somewhat
documented !) instead of hardcoded value (32), but also suggests that there's no
bug in NSPR

Comment 4

[Wan-Teh Chang]

Attachment: Proposed patch

This patch replaces the code that sets _tzname, _daylight,
and _timezone with a _tzset() call.

I'm the author of the code.  Unfortunately I don't remember
the details of the bug and I no longer have access to the
Netscape internal bug database (Bugsplat) we used then.

Looking at the _tzset source code in MS CRT, I see that it
does exactly the same thing (calling GetTimeZoneInformation
and setting _tzname, _daylight, and _timezone).  I also
believe that I didn't know about the _tzset function back
then.  So we should be able to replace the code with a
_tzset() call.

It is possible that the bug has been fixed in a service
pack of Windows NT 4.0 (or a new MS CRT release) so that
this code is no longer necessary.  But it is a lot of work
to find out if this is the case.  So it is safer to continue
to do this workaround, but using only documented interfaces.

The way to review this patch is as follows:
1. Look at the MS CRT source code to verify that _tzset does
what this code does, if the TZ environment variable is not
set.
2. Verify that I correctly replaced the relevant code with
a _tzset() call.
3. Verify that I updated the comment appropriately.

Comment 5

[Wan-Teh Chang]

I just wanted to respond to the statement in comment 2 that
the time zone name in _tzname should be three characters long.
This is wrong, as shown by the following test program run on
a Windows XP system in the GMT time zone, which doesn't have
the TZ environment variable defined.

C:\temp>cat foo.c
#include <time.h>
#include <sys/types.h>
#include <sys/timeb.h>
#include <stdio.h>

int main()
{
    struct _timeb tb;
    _ftime(&tb);
    printf("%s\n", _tzname[0]);
    printf("%s\n", _tzname[1]);
    return 0;
}

C:\temp>cl foo.c
Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 12.00.8804 for 80x86
Copyright (C) Microsoft Corp 1984-1998. All rights reserved.

foo.c
Microsoft (R) Incremental Linker Version 6.00.8447
Copyright (C) Microsoft Corp 1992-1998. All rights reserved.

/out:foo.exe
foo.obj

C:\temp>.\foo.exe
GMT Standard Time
GMT Daylight Time

C:\temp>set TZ
Environment variable TZ not defined

C:\temp>

Comment 6

[Wan-Teh Chang]

I confirm that in MSVC 6.0 and 7.1, the static buffers
_tzname[0] and _tzname[1] point to are 64 characters long.

I don't know why I used 32 in my code.  I guess it's
either because the StandardName and DaylightName members
of the TIME_ZONE_INFORMATION structure are 32 characters
long, or because the static buffers were 32 characters
long in the version of MS CRT I looked at back then.

Comment 7

[Thomas Spear]

To respond to comment 4, I could also try to have the reporter of the bug on
winehq test the patch against the version of wine that was crashing for him
before we made it conform to windows (read: not crash) :-)  Let me know.

To respond to comment 5, I see what you are saying, and noticed something along
those lines myself before the testcase, basically that there are some timezones
(GMT being 1) that dont change the abbreviation when switching from standard to
daylight savings like CST/CDT do..  But since I am not one of the wine coders
per se, I didn't want to try and correct them.  Thanks though.

Mr. Chang, if you would like to have this code tested out under the wine version
that was crashing due to this problem (since we aren't sure it was an actual
bug), please let me know, and I will send the person that reported the problem
on the wine end over here to get the mozilla source and patch.

Also in response to comment 3, I asked the reporter what part of the program it
was that was crashing under wine, and he told me it was the Gecko Runtime
Environment part of the installer itself, not Mozilla.  So I don't know if that
makes any difference in how this particular code would have to be compiled, in
order to test it out, but I thought I would let you all know either way.

Comment 8

[Wan-Teh Chang]

The only information I can still retrieve from the original
Netscape Bugsplat Defect ID 47942 is not much:

=-=-=-=-=-=-=-=-=-=-=-=-=
Under Winnt 4.0, mail's timezone cannot be changed.
Bug#: 47942     Product: Communicator   Version: 4.0b3  Platform: PC
OS/Version: Windows NT  Status: VERIFIED    Severity: major     Priority: P1
Language: all   Resolution: FIXED   Assigned To: wtc    Reported By: xxxxxx
Component: mail/news    Target Fix Version: 4.0b4   Escalation Level:   
Creation Date: MAR-04-1997
URL:
Summary            : Under Winnt 4.0, mail's timezone cannot be changed.
Status Summary:
Description        :
=-=-=-=-=-=-=-=-=-=-=-=-=

I searched in Microsoft Knowledge Base for _ftime, localtime, and
_tzset.  The closest thing I found is KB123717:
http://support.microsoft.com/default.aspx?scid=kb;en-us;123717

I am not sure if it's the same bug.  In any case, I now think we
should just remove that workaround altogether because that bug
must have been fixed by now.

Comment 9

[Wan-Teh Chang]

Attachment: Proposed patch v2

Simply remove the workaround for a very old Windows NT bug.

Comment 10

[Wan-Teh Chang]

Dustin,

Re: comment 7:

I proposed two patches.  Neither one depends on any
undocumented properties of the MS CRT.  So I'm confident
that the crash will be fixed.  It would certainly be
nice to test the patches under wine to verify that.

Since it will take time to get a patch checked into
Mozilla, you may still want to make wine conform to Windows
(i.e., make _tzname[0] and _tzname[1] point to static
buffers 64 characters long) so that current Mozilla
clients won't crash.

Comment 11

[Thomas Spear]

Replies:

Comment #8: Yes, I agree.  The bug was reported in 1997, which was before
Windows 98 was released, so I would assume that the bug was fixed at least by
the time SP5 was released.  If not, I know of very few people (businesses
included) that still use WinNT 4.0.  Most everyone has upgraded at least to
Win2k, if not switched completely to Linux these days.

Comment #10: That is not a problem.  Soon after the original reporter reported
the crash in wine, the developers found both, the problem in wine, and the
problem in Mozilla.  They fixed the one in wine, and asked me to go ahead and
report the Mozilla one to you guys.  Thanks so much for your help.

As I said before, once you get the patch checked into Mozilla, I will talk to
the original reporter and have him revert to the version of wine that acted up
(caused the crash), since it was probably the _proper_ way of doing things (as
opposed to the braindead M$ way of doing things lol), just to see if the new
patch causes any problems..

If someone could please, since I dont monitor the mozdev mailing lists, post a
note here when the patch does get committed, it would be appreciated.

Thanks again for all of your hard work, and I look forward to working with
everyone here in the future.

Comment 12

[Wan-Teh Chang]

Comment on attachment 182005 [details] [diff] [review]
Proposed patch v2

Requesting NSPRPUB_PRE_4_2_CLIENT_BRANCH checkin approval
for mozilla1.8b2 and aviary1.1a.

This patch removes an ugly workaround for a very old Windows
NT 4.0 bug.  We believe that bug must have been fixed in a
Windows NT 4.0 service pack.  The workaround relied on
undocumented properties of the implementation of msvcrt.dll
(the Standard C library).

Comment 13

[Wan-Teh Chang]

Patch v2 checked in on the NSPR trunk for NSPR 4.6.

Checking in ntmisc.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntmisc.c,v  <--  ntmisc.c
new revision: 3.19; previous revision: 3.18
done
Checking in ntthread.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntthread.c,v  <--  ntthread.c
new revision: 3.18; previous revision: 3.17
done
Checking in w95thred.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95thred.c,v  <--  w95thred.c
new revision: 3.15; previous revision: 3.14
done

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 182005 [details] [diff] [review]
Proposed patch v2

a=asa

Comment 15

[Wan-Teh Chang]

Patch v2 checked in on the NSPRPUB_PRE_4_2_CLIENT_BRANCH
for Mozilla 1.8 Beta 2.

Checking in ntmisc.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntmisc.c,v  <--  ntmisc.c
new revision: 3.15.4.4; previous revision: 3.15.4.3
done
Checking in ntthread.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntthread.c,v  <--  ntthread.c
new revision: 3.13.4.5; previous revision: 3.13.4.4
done
Checking in w95thred.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95thred.c,v  <--  w95thred.c
new revision: 3.9.4.6; previous revision: 3.9.4.5
done