8348323: Corrupted timezone string in JVM crash log #23246

dhanalla · 2025-01-23T01:10:05Z

Add a error check to "wcstombs" otherwise the "buf" prints junk or corrupted string.

Test performed:

"jcmd < jvm pid > VM.info" for different JVM versions

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8348323: Corrupted timezone string in JVM crash log (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Amit Kumar (@offamitkumar - Committer)
Kevin Walls (@kevinjwalls - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23246/head:pull/23246
$ git checkout pull/23246

Update a local copy of the PR:
$ git checkout pull/23246
$ git pull https://git.openjdk.org/jdk.git pull/23246/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23246

View PR using the GUI difftool:
$ git pr show -t 23246

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23246.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-23T01:11:01Z

👋 Welcome back dhanalla! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-01-23T01:11:21Z

@dhanalla This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8348323: Corrupted timezone string in JVM crash log

Reviewed-by: dholmes, amitkumar, kevinw

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 22 new commits pushed to the master branch:

039e73f: 8346736: Java Security Standard Algorithm Names spec should include key algorithm names
aba60a9: 8189441: Define algorithm names for keys derived from KeyAgreement
03106eb: 8344119: CUPSPrinter does not respect PostScript printer definition specification in case of reading ImageableArea values from PPD files
ad01dfb: 8346920: Serial: Support allocation in old generation when heap is almost full
1d8ccb8: 8342465: Improve API documentation for java.lang.classfile
7d6055a: 8348429: Update cross-compilation devkits to Fedora 41/gcc 13.2
f1e0797: 8348586: Optionally silence make warnings about non-control variables
ffeb9b5: 8342807: Update links in java.base to use https://
afcc2b0: 8348562: ZGC: segmentation fault due to missing node type check in barrier elision analysis
175e58b: 8332980: [IR Framework] Add option to measure IR rule processing time
... and 12 more: https://git.openjdk.org/jdk/compare/bef5f56544ded9307e5f1095bd42ae46fdc2adda...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @offamitkumar, @kevinjwalls) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-01-23T01:11:56Z

@dhanalla The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-01-23T01:16:33Z

Webrevs

01: Full - Incremental (0a087b38)
00: Full (9cda63e7)

dholmes-ora

Change looks good, but I'm very curious as to why wcstombs is actually failing i.e. where the bad input is coming from?

dhanalla · 2025-01-23T19:15:26Z

Change looks good, but I'm very curious as to why wcstombs is actually failing i.e. where the bad input is coming from?

Thank you @dholmes-ora, I’m not sure about the locale of the timezone input, but according to the crash report shared by one of our clients, wcstombs seems to be failing to convert wide character to a multibyte string.

dhanalla · 2025-01-23T19:17:17Z

/integrate

openjdk · 2025-01-23T19:17:43Z

@dhanalla
Your change (at version 9cda63e) is now ready to be sponsored by a Committer.

dholmes-ora · 2025-01-24T06:40:00Z

/reviewers 2

dholmes-ora · 2025-01-24T06:40:23Z

@dhanalla all hotspot changes require two reviews

openjdk · 2025-01-24T06:41:10Z

@dholmes-ora
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

offamitkumar

If wcstombs fails, it will return -1 of size_t type. So change looks correct.

offamitkumar · 2025-01-24T07:49:50Z

I guess you should update copyright header year also.

kevinjwalls · 2025-01-24T09:21:35Z

Hi,
It's not random corruption, as it's the same across different machines.
We are getting timezone printing wrong, and this is I think a way of not printing the timezone.

It would be great to have more info/examples in the JBS issue.
The one example in the JBS issue contains "-eur" so perhaps part of a timezone name in Europe...

What timezone are the machines actually set to?
Is there a TZ variable set? like in JDK-8340465

openjdk · 2025-01-24T20:46:05Z

@dhanalla this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout Corrupted-Timezone-String
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

dhanalla · 2025-01-24T21:39:02Z

Hi, It's not random corruption, as it's the same across different machines. We are getting timezone printing wrong, and this is I think a way of not printing the timezone.

It would be great to have more info/examples in the JBS issue. The one example in the JBS issue contains "-eur" so perhaps part of a timezone name in Europe...

What timezone are the machines actually set to? Is there a TZ variable set? like in JDK-8340465

Thanks @kevinjwalls, One of our client's crash collection systems has captured this report; however, we currently do not have information regarding the locale or timezone of the user at the time the crash occurred.

dhanalla · 2025-01-24T21:41:57Z

I guess you should update copyright header year also.

Thanks @offamitkumar, Merging with the latest version of this file updated the year in the copyright header.

kevinjwalls · 2025-01-25T09:56:45Z

Thanks @kevinjwalls, One of our client's crash collection systems has captured this report; however, we currently do not have information regarding the locale or timezone of the user at the time the crash occurred.

OK thanks. It seems like there is more to the mystery that is not yet solved. 8-)

I don't disagree that we should check wcstombs' return value. When we look at this kind of thing in future it will be no timezone printed that means we have hit an existing problem fetching a valid timezone.

offamitkumar · 2025-01-25T11:10:47Z

When we look at this kind of thing in future it will be no timezone printed that means we have hit an existing problem fetching a valid timezone.

Just a thought, can't use this logic to make it more clear ? :

diff --git a/src/hotspot/share/runtime/os.cpp b/src/hotspot/share/runtime/os.cpp
index 094d68e1a84..aea9aee5adf 100644
--- a/src/hotspot/share/runtime/os.cpp
+++ b/src/hotspot/share/runtime/os.cpp
@@ -1213,8 +1213,12 @@ void os::print_date_and_time(outputStream *st, char* buf, size_t buflen) {
     wchar_t w_buf[80];
     size_t n = ::wcsftime(w_buf, 80, L"%Z", &tz);
     if (n > 0) {
-      ::wcstombs(buf, w_buf, buflen);
-      st->print("Time: %s %s", timestring, buf);
+      bool success = (::wcstombs(buf, w_buf, buflen) != (size_t)-1);
+      if (success) {
+        st->print("Time: %s %s", timestring, buf);
+      } else {
+        st->print("Time: %s (Error: Failed to convert timezone to multi-byte string)", timestring);
+      }
     } else {
       st->print("Time: %s", timestring);
     }

dhanalla · 2025-01-27T18:34:34Z

When we look at this kind of thing in future it will be no timezone printed that means we have hit an existing problem fetching a valid timezone.

Just a thought, can't use this logic to make it more clear ? :

diff --git a/src/hotspot/share/runtime/os.cpp b/src/hotspot/share/runtime/os.cpp
index 094d68e1a84..aea9aee5adf 100644
--- a/src/hotspot/share/runtime/os.cpp
+++ b/src/hotspot/share/runtime/os.cpp
@@ -1213,8 +1213,12 @@ void os::print_date_and_time(outputStream *st, char* buf, size_t buflen) {
     wchar_t w_buf[80];
     size_t n = ::wcsftime(w_buf, 80, L"%Z", &tz);
     if (n > 0) {
-      ::wcstombs(buf, w_buf, buflen);
-      st->print("Time: %s %s", timestring, buf);
+      bool success = (::wcstombs(buf, w_buf, buflen) != (size_t)-1);
+      if (success) {
+        st->print("Time: %s %s", timestring, buf);
+      } else {
+        st->print("Time: %s (Error: Failed to convert timezone to multi-byte string)", timestring);
+      }
     } else {
       st->print("Time: %s", timestring);
     }

Thanks @offamitkumar,
This logic will result in the time not being printed, whereas we were only intending to skip the time zone string.

dhanalla · 2025-01-27T18:35:20Z

/integrate

openjdk · 2025-01-27T18:36:00Z

@dhanalla
Your change (at version 0a087b3) is now ready to be sponsored by a Committer.

offamitkumar · 2025-01-28T02:05:15Z

This logic will result in the time not being printed

why is that ? I think timezone info is coming from here:

    wchar_t w_buf[80];
    size_t n = ::wcsftime(w_buf, 80, L"%Z", &tz);

which then we try to convert to multi byte string. There are two cases in my solution. With success we will print both time and timezone(buf) as before , in case of failure we should be able to print timestring with short error message.

kevinjwalls · 2025-01-28T09:58:30Z

Hi,
Would it be OK to proceed with this as it stands?

I understand the idea of showing the error message. Concerned that printed in-line it makes the line very long, may obscure the elapsed time in some situations. This hs_err line may be parsed by tools, so adding a free form error message may be harder to parse than no timezone.

Not printing a timezone will mean we couldn't decode one.

The case I noted in JDK-8340465 was that setting a TZ variable may contain something that wasn't recognised in Windows. That's why I asked if we knew what TZ was in the case here.

There might be more to do here in another change, but it wasn't clear if we can ignore TZ, and use the Windows api only, or if that creates other problems.

offamitkumar · 2025-01-28T10:02:02Z

Concerned that printed in-line it makes the line very long, may obscure the elapsed time in some situations. This hs_err line may be parsed by tools, so adding a free form error message may be harder to parse than no timezone.

Aah, I see. That makes sense. I am fine with current changes as well.

kevinjwalls · 2025-01-28T11:31:17Z

Thanks.
@dhanalla If you ever find out what TZ/locale the original machine had, it would be great to add a note in JDK-8340465.

kevinjwalls · 2025-01-28T11:31:22Z

/sponsor

openjdk · 2025-01-28T11:32:03Z

Going to push as commit 2bef5b4.
Since your change was applied there have been 32 commits pushed to the master branch:

3a8680e: 8347058: When automatically translating the page to pt-br, all CSS styling disappears
a4942a2: 8348286: [AIX] clang 17 introduces new warning Wtentative-Definitions which produces Build errors
558255a: 8328919: Add BodyHandlers / BodySubscribers methods to handle excessive server input
1f74caa: 8348401: [IR Framework] PrintTimes should not require verbose
f71541c: 8344976: Remove the jmx.invoke.getters compatibility property
2537a05: 8348384: RISC-V: Disable auto-enable Vector on buggy kernels
46f48e4: 8348515: Add docs for -XX:AOT* options in java man pages
1916a77: 8346923: MetaspaceShared base calculation may cause overflow in align_up
21feef3: 8348239: SA does not know about DeoptimizeObjectsALotThread
8cc1304: 8287788: Implement a better allocator for downcalls
... and 22 more: https://git.openjdk.org/jdk/compare/bef5f56544ded9307e5f1095bd42ae46fdc2adda...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-28T11:32:27Z

@kevinjwalls @dhanalla Pushed as commit 2bef5b4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

dhanalla · 2025-02-03T21:38:19Z

/backport jdk21u-dev

openjdk · 2025-02-03T21:39:56Z

@dhanalla the backport was successfully created on the branch backport-dhanalla-2bef5b4a-master in my personal fork of openjdk/jdk21u-dev. To create a pull request with this backport targeting openjdk/jdk21u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 2bef5b4a from the openjdk/jdk repository.

The commit being backported was authored by Dhamoder Nalla on 28 Jan 2025 and was reviewed by David Holmes, Amit Kumar and Kevin Walls.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u-dev:

$ git fetch https://github.com/openjdk-bots/jdk21u-dev.git backport-dhanalla-2bef5b4a-master:backport-dhanalla-2bef5b4a-master
$ git checkout backport-dhanalla-2bef5b4a-master
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u-dev.git backport-dhanalla-2bef5b4a-master

⚠️ @dhanalla You are not yet a collaborator in my fork openjdk-bots/jdk21u-dev. An invite will be sent out and you need to accept it before you can proceed.

dhanalla · 2025-02-25T20:27:06Z

/backport jdk24u

openjdk · 2025-02-25T20:28:00Z

@dhanalla the backport was successfully created on the branch backport-dhanalla-2bef5b4a-master in my personal fork of openjdk/jdk24u. To create a pull request with this backport targeting openjdk/jdk24u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 2bef5b4a from the openjdk/jdk repository.

The commit being backported was authored by Dhamoder Nalla on 28 Jan 2025 and was reviewed by David Holmes, Amit Kumar and Kevin Walls.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk24u:

$ git fetch https://github.com/openjdk-bots/jdk24u.git backport-dhanalla-2bef5b4a-master:backport-dhanalla-2bef5b4a-master
$ git checkout backport-dhanalla-2bef5b4a-master
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk24u.git backport-dhanalla-2bef5b4a-master

⚠️ @dhanalla You are not yet a collaborator in my fork openjdk-bots/jdk24u. An invite will be sent out and you need to accept it before you can proceed.

Fix to Timezone string corruption

9cda63e

openjdk bot added the rfr Pull request is ready for review label Jan 23, 2025

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jan 23, 2025

dholmes-ora approved these changes Jan 23, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 23, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 23, 2025

openjdk bot removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Jan 24, 2025

offamitkumar approved these changes Jan 24, 2025

View reviewed changes

openjdk bot added sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Jan 24, 2025

update year in copyright header

540803d

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Jan 24, 2025

Merge branch 'master' into Corrupted-Timezone-String

0a087b3

openjdk bot added the ready Pull request is ready to be integrated label Jan 24, 2025

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Jan 24, 2025

kevinjwalls approved these changes Jan 25, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 27, 2025

openjdk bot added the integrated Pull request has been integrated label Jan 28, 2025

openjdk bot closed this Jan 28, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jan 28, 2025

8348323: Corrupted timezone string in JVM crash log #23246

8348323: Corrupted timezone string in JVM crash log #23246

Uh oh!

Conversation

dhanalla commented Jan 23, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jan 23, 2025

Uh oh!

openjdk bot commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jan 23, 2025

Uh oh!

mlbridge bot commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhanalla commented Jan 23, 2025

Uh oh!

dhanalla commented Jan 23, 2025

Uh oh!

openjdk bot commented Jan 23, 2025

Uh oh!

dholmes-ora commented Jan 24, 2025

Uh oh!

dholmes-ora commented Jan 24, 2025

Uh oh!

openjdk bot commented Jan 24, 2025

Uh oh!

offamitkumar left a comment

Choose a reason for hiding this comment

Uh oh!

offamitkumar commented Jan 24, 2025

Uh oh!

kevinjwalls commented Jan 24, 2025

Uh oh!

openjdk bot commented Jan 24, 2025

Uh oh!

dhanalla commented Jan 24, 2025

Uh oh!

dhanalla commented Jan 24, 2025

Uh oh!

kevinjwalls commented Jan 25, 2025

Uh oh!

offamitkumar commented Jan 25, 2025

Uh oh!

dhanalla commented Jan 27, 2025

Uh oh!

dhanalla commented Jan 27, 2025

Uh oh!

openjdk bot commented Jan 27, 2025

Uh oh!

offamitkumar commented Jan 28, 2025

Uh oh!

kevinjwalls commented Jan 28, 2025

Uh oh!

offamitkumar commented Jan 28, 2025

Uh oh!

kevinjwalls commented Jan 28, 2025

Uh oh!

kevinjwalls commented Jan 28, 2025

Uh oh!

openjdk bot commented Jan 28, 2025

Uh oh!

openjdk bot commented Jan 28, 2025

Uh oh!

dhanalla commented Feb 3, 2025

Uh oh!

openjdk bot commented Feb 3, 2025

Uh oh!

dhanalla commented Feb 25, 2025

dhanalla commented Jan 23, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 23, 2025 •

edited

Loading

mlbridge bot commented Jan 23, 2025 •

edited

Loading

dholmes-ora left a comment •

edited

Loading