Bug 58286

Summary: Crash in jk-status on Windows (when producing HTML output)
Product: Tomcat Connectors Reporter: Rainer Jung <rainer.jung>
Component: CommonAssignee: Tomcat Developers Mailing List <dev>
Severity: normal CC: chinoygupta
Priority: P2    
Version: 1.2.41   
Target Milestone: ---   
Hardware: All   
OS: All   

Description Rainer Jung 2015-08-26 18:42:34 UTC
The fix for BZ 54177 introduced a new problem on the Windows platform. The use of strftime() with the format pattern %z works well on Unix/Linux, but %z on Windows has a totally different meaning. It doesn't return a numeric timezone info, but instead the full time zone name. This results in the same non-well formed XML problem as in BZ 54177, but furthermore when formatting the time stamp in jk-status for HTML output it results in a crash. This should happen for mod_jk and for the ISAPI redirector. Apart from Windows there might also be a problem on Netware (untested).

The problem should not happen, when calling jk-status with the query string ?mime=txt, ?mime=prop or ?mime=xml. Only the default (equivalent to ?mime=html) is affected.

Since the time zone info is not critical, the simplest short term mitigation is removing "%z" from the strftime format string before building:

Index: native/common/jk_status.c
--- native/common/jk_status.c   (revision 1697988)
+++ native/common/jk_status.c   (working copy)
@@ -241,9 +241,9 @@
 #define JK_STATUS_WAIT_AFTER_UPDATE        "3"
 #define JK_STATUS_REFRESH_DEF              "10"
 #define JK_STATUS_ESC_CHARS                ("<>?\"")
-#define JK_STATUS_TIME_FMT_HTML            "%Y-%m-%d %H:%M:%S %z"
+#define JK_STATUS_TIME_FMT_HTML            "%Y-%m-%d %H:%M:%S"
 #define JK_STATUS_TIME_FMT_TEXT            "%Y%m%d%H%M%S"
-#define JK_STATUS_TIME_FMT_TZ              "%z"
+#define JK_STATUS_TIME_FMT_TZ              ""
 #define JK_STATUS_TIME_BUF_SZ              (30)

 #define JK_STATUS_HEAD                     "<?xml version=\"1.0\" encoding=\"ISO-8859-1\"?>\n" \

Any broken binary should be "fixable" as a workaound by editing and overwriting the two occurences of %z in the binary file with spaces. Do not remove the %z, just replace by two spaces in both places where it occurs. make sure to not change any other bytes in the binary.

The fix for the next version will be to use specific timezone formatting for Windows. Probably it will be based on _get_timezone(). Netware will need to be checked.
Comment 1 Christopher Schultz 2015-08-27 14:39:29 UTC
Looking at the manual pages on a Linux and BSD system, it does appear that the %z format specifier is indeed an "extension" to C90. The Linux manual page specifically identifies it as a "Single UNIX Specification" extension:

       %z     The  +hhmm  or  -hhmm  numeric  timezone  (that is, the hour and
              minute offset from UTC). (SU)

The "(SU)" notation indicates that it's a "Single Unix Specification" extension. So it's not surprising that it's not supported on Microsoft Windows.

Having the time zone information in a timestamp is pretty important. C90 specifies the "%Z" format specifier. Can we use that instead?

What is the root cause of the crash? stfrtime takes a buffer-size argument and should not overrun it. Is this really a bug in the win32 POSIX library?
Comment 2 Rainer Jung 2015-08-27 19:05:50 UTC
%z (lower case) was first added (standardized) in "IEEE Std 1003.1, 2004 Edition" aka SUSv3 (after including issue 6).



%Z (upper case) was what we used before. As I wrote, it was changed in order to fix BZ 54177. Concerning the importantce of the tz info, I dont fully agree. This is "just" jk-status output. So it would be nice to have but rating all the stuff that jk-status shows, IMHO the current tz is one of the least important things.

Looking at the available APIs on Windows I tend to drop the tz info for that platform. BZ 54177 suggests we should stick to numeric representation (at least to XML safe stuff) and I invite you to look at the apr file "time/win32/time.c" how tm_gmtoff is calculated in apr_time_exp_lt(). That tm_gmtoff is what httpd uses to print the tz offset in its access log. At least for me that's not a road I'd like to go "only" for tz. Any other suggestions are welcome, but short term it seems best to remove and stay on the safe side.

About the root cause of the crash: httpd+mod_jk should not crash due to buffer overrun and in a simple standalone test it does not and simply returns without formatting the time stamp. In the debugger, I can see that it crashes in strftime and only if "%z" is used and also not when "%z" is the only thing to format. In addition one can see, that the buffer would not be big enough to accommodate the full formatted timestamp. Why it crashes in the module and not in the standalone example is not known to me. I don't have PDB files for all components, so it is a bit hard to debug further. But the crash definitely happens in strftime() and is definitely related to "%z" in combination with other format chars in the pattern and a too small buffer. And we definitely don't want to %z on Windows because its output (long textual name of time zone) is not what we want.
Comment 3 Rainer Jung 2015-08-28 12:17:41 UTC
Fixed in r1698316, will be part of 1.2.42.

I found a way to get the timezone offset even on Windows without too much hassle. Not very nice code though.

Tested in combination with httpd 2.4 on Windows (only for my local timezone).

Not sure, whether there is still a problem on Netware.

According to http://www.novell.com/documentation/developer/libc/libc_vol2/data/sdk1810.html, %z should work when using libc.

According to http://www.novell.com/documentation/developer/clib/prog_enu/data/sdk1810.html, %z should not be available when using clib.

Not sure, which variant of Netware - libc or clib - we actually support.

Clsing, but can be reopened, if we need a fix on Netware.
Comment 4 Chinoy Gupta 2015-09-02 08:23:36 UTC
Is 1.2.42 release planned in near future or should we integrate this fix now?
Comment 5 Rainer Jung 2015-09-02 09:05:44 UTC
If you can easily integrate now, I suggest you do so.

There are currently only two post 1.2.41 fixes, this one here and one about atomics on some niche platforms (not Windows, not Linux 64 Bit). This one here affects all Windows users, but one of the frequently used providers for Windows binaries (ApacheLounge) has now included the fix in his 1.2.41 build. So it seems there is no high pressure for a release.

I worked a bit on our own Windows build system, so I hope we can finally provide good quality binaries for Windows for 1.2.42. I want to update the docs and probably have a look at some of the old outstanding ISAPI redirector bugs. So personally I don't plan a release very soon (not this month, maybe much later depending on what happens in the meantime).
Comment 6 Chinoy Gupta 2015-09-02 09:10:17 UTC
Thanks for the prompt reply. Based on your comment, it might be some time before  the next release comes out. So I will look into integrating it now only.
Comment 7 Rainer Jung 2015-09-09 12:54:43 UTC
Added r1701991 to the fix. The timezone offset was wrong for DST.