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

Remove timegm() compat #1056

Merged
merged 2 commits into from
May 25, 2024
Merged

Remove timegm() compat #1056

merged 2 commits into from
May 25, 2024

Conversation

botovq
Copy link
Contributor

@botovq botovq commented May 25, 2024

Now that all uses of gmtime_r() and timegm() have been converted to OPENSSL_gmtime() and OPENSSL_timegm(), this is no longer needed.

Now that all uses of gmtime_r() and timegm() have been converted to
OPENSSL_gmtime() and OPENSSL_timegm(), this is no longer needed.
@joshuasing
Copy link
Member

I think you may have already seen, however the asn1time test appears to be failing on linux/arm32.
Could this log output be related?

asn1time.c: In function ‘asn1_gentime_test’:
asn1time.c:311:27: warning: passing argument 2 of ‘OPENSSL_timegm’ from incompatible pointer type [-Wincompatible-pointer-types]
 311 |  if (!OPENSSL_timegm(&tm, &a)) {
     |                           ^~
     |                           |
     |                           int64_t * {aka long long int *}
In file included from asn1time.c:20:
../include/openssl/posix_time.h:48:49: note: expected ‘time_t *’ {aka ‘long int *’} but argument is of type ‘int64_t *’ {aka ‘long long int *’}
  48 | int OPENSSL_timegm(const struct tm *tm, time_t *out);
     |                                         ~~~~~~~~^~~

@botovq
Copy link
Contributor Author

botovq commented May 25, 2024

Thanks. Yes, it's part of what's wrong here, I reverted that change I made.

But there's more. The check I tried to adapt also can't fail, as all the times in asn1_gentime_tests are long before the epoch wraps, so it makes no sense in the first place...

@joshuasing
Copy link
Member

Ah, okay. That's interesting. I reran the failed CI, which seems to have pulled the "Revert previous" commit from libressl/openbsd, and it has passed.

@botovq
Copy link
Contributor Author

botovq commented May 25, 2024

It's not ready yet, the asn1time test now again has timegm() apparently none of our CI systems are needing the compat implementation though.

@botovq
Copy link
Contributor Author

botovq commented May 25, 2024

@joshuasing As you can see, arm32 is completely busted once you add a test case that tickles it.

FAIL: test 3 - times don't match, expected -2147483648 got -1
UTCTIME tests...
TIME tests...
ASN1_TIME_compare tests...
20161208193400Z vs. 20380119031408Z: want 1, got -1
20161208193400Z vs. -2147483648: want -1, got 1
20161208193400Z vs. -2147483648: want -1, got 1
19700101000000Z vs. 20380119031408Z: want 1, got -1
19700101000000Z vs. -2147483648: want -1, got 1
19700101000000Z vs. -2147483648: want -1, got 1
20150923032700Z vs. 20380119031408Z: want 1, got -1
20150923032700Z vs. -2147483648: want -1, got 1
20150923032700Z vs. -2147483648: want -1, got 1
20380119031408Z vs. 20161208193400Z: want -1, got 1
...

@botovq botovq force-pushed the timegm branch 2 times, most recently from 4025877 to 81a1e00 Compare May 25, 2024 18:35
The RFC 5280 test now passes also with small time_t. The ASN.1 time
test now has a test case that makes it fail for small time t. In that
case use a wrapper script that prins why the test is expected to fail
and makes the test suite fail if it passes.
@botovq botovq merged commit 8f03828 into libressl:master May 25, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants