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

asn1_string_to_time_t: Use timegm on FreeBSD. #17765

Closed
wants to merge 1 commit into from

Conversation

bsdjhb
Copy link
Contributor

@bsdjhb bsdjhb commented Feb 24, 2022

FreeBSD does not provide a global timezone variable containing the
offset to UTC. Instead, FreeBSD's libc includes a legacy timezone
function dating back to Version 7 AT&T UNIX. As a result,
asn1_string_to_time_t currently fails to compile on FreeBSD as it
subtracts a function from a time_t value:

../crypto/asn1/a_time.c:625:37: error: invalid operands to binary expression ('time_t' (aka 'long') and 'char *(int, int)')
timestamp_utc = timestamp_local - timezone;
~~~~~~~~~~~~~~~ ^ ~~~~~~~~
1 error generated.

However, FreeBSD's libc does include a non-standard (but widely
available) timegm function which converts a struct tm directly to a
UTC time_t value. Use this on FreeBSD instead of mktime.

Checklist
  • documentation is added or updated
  • tests are added or updated

FreeBSD does not provide a global timezone variable containing the
offset to UTC.  Instead, FreeBSD's libc includes a legacy timezone
function dating back to Version 7 AT&T UNIX.  As a result,
asn1_string_to_time_t currently fails to compile on FreeBSD as it
subtracts a function from a time_t value:

../crypto/asn1/a_time.c:625:37: error: invalid operands to binary expression ('time_t' (aka 'long') and 'char *(int, int)')
    timestamp_utc = timestamp_local - timezone;
                    ~~~~~~~~~~~~~~~ ^ ~~~~~~~~
1 error generated.

However, FreeBSD's libc does include a non-standard (but widely
available) timegm function which converts a struct tm directly to a
UTC time_t value.  Use this on FreeBSD instead of mktime.
@paulidale paulidale added approval: review pending This pull request needs review by a committer branch: 3.0 Merge to openssl-3.0 branch branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug labels Feb 24, 2022
@paulidale
Copy link
Contributor

Question: should 1.1.1 get this fix too?

@kaduk
Copy link
Contributor

kaduk commented Feb 24, 2022

AFAICT the function was only added by #17645 and is not present on 1.1.1 or even 3.0.

@mattcaswell mattcaswell removed the branch: 3.0 Merge to openssl-3.0 branch label Feb 24, 2022
@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 24, 2022
@bsdjhb
Copy link
Contributor Author

bsdjhb commented Feb 24, 2022

BTW, I had a thought this morning: even though timegm isn't standardized, you might consider using it when it is available (which it should be on BSD's, Linux, and macOS at least). The existing logic of using mktime + subtraction against timezone has some edge cases with DST transitions that are correctly handled by timegm (e.g. if the time in tm is in a different part of the year as far as DST rules as when tzset was last called then the result can be incorrect). This can occur even for requests for the current time in a long-running daemon whose runtime spans DST transitions. A portable alternative to using timegm is to use tzset to temporarily override TZ while calling mktime. That alternative has other issues though (not thread-safe and some systems leak memory in setenv due to the fact that pointers returned from getenv are supposed to be valid "forever").

@mattcaswell
Copy link
Member

If you can suggest a more encompassing "ifdef" for the USE_TIMEGM define then I'd be open to include it, i.e. basically use mktime on a platform if we don't know for sure that timegm is supported. Probably want to include a comment briefly explaining why timegm is preferred where available. I'd do that in a separate PR though since this one has already achieved approval.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@bsdjhb
Copy link
Contributor Author

bsdjhb commented Feb 25, 2022

I think a separate PR makes sense. I will see if I can come up with a compile-time configure test for timegm.

@mattcaswell mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Feb 25, 2022
@t8m
Copy link
Member

t8m commented Feb 28, 2022

I will see if I can come up with a compile-time configure test for timegm.

We do not do compile-time configure tests currently. I'd suggest instead just enabling this on glibc and macos.

@mattcaswell
Copy link
Member

Pushed. Thanks.

@mattcaswell mattcaswell closed this Mar 3, 2022
openssl-machine pushed a commit that referenced this pull request Mar 3, 2022
FreeBSD does not provide a global timezone variable containing the
offset to UTC.  Instead, FreeBSD's libc includes a legacy timezone
function dating back to Version 7 AT&T UNIX.  As a result,
asn1_string_to_time_t currently fails to compile on FreeBSD as it
subtracts a function from a time_t value:

../crypto/asn1/a_time.c:625:37: error: invalid operands to binary expression ('time_t' (aka 'long') and 'char *(int, int)')
    timestamp_utc = timestamp_local - timezone;
                    ~~~~~~~~~~~~~~~ ^ ~~~~~~~~
1 error generated.

However, FreeBSD's libc does include a non-standard (but widely
available) timegm function which converts a struct tm directly to a
UTC time_t value.  Use this on FreeBSD instead of mktime.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #17765)
@bsdjhb bsdjhb deleted the timegm branch March 4, 2022 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants