Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix integer overflow when parsing SOURCE_DATE_EPOCH #186

Closed
wants to merge 2 commits into from

Conversation

mizdebsk
Copy link
Contributor

Parsing SOURCE_DATE_EPOCH environment variable by Tstamp leads to integer overflow and produces wrong result.

The following example illustrates that:

<tstamp/>
<echo>TODAY is ${TODAY}</echo>

This produces the following output:

[tstamp] Honouring environment variable SOURCE_DATE_EPOCH which has been set to 1650585600
[echo] TODAY is January 16 1970

Which is incorrect, because timestamp 1650585600 corresponds to April 22 2022.

@jaikiran
Copy link
Member

Hello @mizdebsk, thank you for contributing this fix.

Would you be interested in adding a test case for this change so that this doesn't regress in future. We have an existing test class for this task which you can add a new test to here https://github.com/apache/ant/blob/master/src/tests/junit/org/apache/tools/ant/taskdefs/TStampTest.java.

If you won't be able to contribute the test, that's fine too. Just let us know.

@mizdebsk
Copy link
Contributor Author

Sure, I will add a test case.

@mizdebsk
Copy link
Contributor Author

I've added a test case that demonstrates the issue.
Test case is more complex than the actual fix.
Does it mean I have to sign Apache CLA? I have not signed it yet.

The test case is implemented as an AntUnit test. JUnit test case would not work because the test requires setting environment variable SOURCE_DATE_EPOCH, which must be done by forking JVM - the is no setenv in Java.

Without the fix the test fails in the following way:

Target: testSourceDateEpoch  FAILED
    at line 107, column 77
    Message: Expected 'TODAY is April 22 2022' but was 'TODAY is January 16 1970'
    took 0.166 sec

And it passes with the fix:

Target: testSourceDateEpoch took 0.164 sec

@kleini
Copy link

kleini commented May 20, 2022

We're having an issue, with this, too.

[tstamp] Honouring environment variable SOURCE_DATE_EPOCH which has been set to 1651903417
Date of 12/12/1969 07:10 PM results in negative milliseconds value relative to epoch (January 1, 1970, 00:00:00 GMT).

@jaikiran
Copy link
Member

I've added a test case that demonstrates the issue.
Test case is more complex than the actual fix.
Does it mean I have to sign Apache CLA? I have not signed it yet.

Hello @mizdebsk, I just saw this message. The contributor guidelines don't state that you are expected to sign the CLA for the contribution https://infra.apache.org/contributors.html. However, I'll check internally once.

Thank you for that test case. It looks fine to me. I'll run it once internally and merge it (after the CLA decision).

@jaikiran jaikiran closed this in babd1c4 May 21, 2022
@jaikiran
Copy link
Member

I've received confirmation that CLA is not a must. I've also tested your changes in this PR and they look good and I've now merged it. Thank you for this fix and the test case and apologies for the delay in merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants