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.

Sign up for GitHub

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

Jump to bottom

OPENSSL_strcasecmp build, cleanup, and initialization fixes #18282

Closed
wants to merge 8 commits into from
Closed

OPENSSL_strcasecmp build, cleanup, and initialization fixes #18282

wants to merge 8 commits into from

Conversation

t8m
Copy link
Member

@t8m t8m commented May 10, 2022

This PR encompasses cleanups, build, and initialization fixes for the OPENSSL_strcasecmp related fix.

I recommend reviewing by individual commits.

We will need similar PR for 3.0 but that will require some backporting effort because the patches won't apply cleanly. In the end however the final state on 3.0 should be exactly the same.

@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug labels May 10, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label May 10, 2022
@t8m t8m force-pushed the openssl-strcasecmp-init branch from a504e44 to 07c3248 Compare May 10, 2022 15:30
@t8m t8m force-pushed the openssl-strcasecmp-init branch from 07c3248 to 2456d53 Compare May 10, 2022 15:34
@beldmit
Copy link
Member

beldmit commented May 10, 2022

I like this approach. And you managed to deduplicate the initialization (failed by me).

I'm not sure that TANDEM fix isn't worth a separate PR, BTW

I'll make a more thorough review when tests are passed.

@rsbeckerca
Copy link
Contributor

I like this approach. And you managed to deduplicate the initialization (failed by me).

I'm not sure that TANDEM fix isn't worth a separate PR, BTW

I'll make a more thorough review when tests are passed.

Considering this PR is to fix the breakage originally introduced, it might be a bit of overkill. However, if needed, I can put the PR together once this is merged. We are unable to move our CI forward without both, however, which continues to fail on openssl-3.0 and master.

beldmit
beldmit approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@beldmit beldmit removed the approval: otc review pending This pull request needs review by an OTC member label May 10, 2022
@paulidale paulidale 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 May 10, 2022
@t8m t8m mentioned this pull request May 11, 2022
Closed
@t8m
Copy link
Member Author

t8m commented May 11, 2022

One small fixup pushed @beldmit @paulidale still OK?

@beldmit
Copy link
Member

beldmit commented May 11, 2022

Still LGTM

@beldmit
Copy link
Member

beldmit commented May 11, 2022

Don't need to reset the timer, I think

@rsbeckerca
Copy link
Contributor

I launched a build/test cycle for this branch and will report back when done.

@rsbeckerca rsbeckerca mentioned this pull request May 11, 2022
Closed
@rsbeckerca
Copy link
Contributor

Is this PR based on the openssl-3.0 branch or master? I am seeking breakages that have previously only shown up on master not openssl-3.0.

@paulidale
Copy link
Contributor

Still good.

@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.

paulidale
paulidale reviewed May 12, 2022
View reviewed changes
crypto/init.c
@@ -498,6 +484,9 @@ int OPENSSL_init_crypto(uint64_t opts, const OPENSSL_INIT_SETTINGS *settings)
aloaddone = 1;
}

if (!ossl_init_casecmp())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of wondering about the need for this. Why not let it lazily init itself? This will simplify things a little.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer explicit initialization to lazy one. I want to keep it.

@t8m t8m 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 May 13, 2022
@t8m
Copy link
Member Author

t8m commented May 13, 2022

Merged to master branch. Thank you for your reviews.

@t8m t8m closed this May 13, 2022
openssl-machine pushed a commit that referenced this pull request May 13, 2022
@t8m
int_ctx_new(): Revert extra OPENSSL_init_crypto() call
615525b 
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #18282)
openssl-machine pushed a commit that referenced this pull request May 13, 2022
@t8m
Fix build on OPENSSL_SYS_TANDEM and older POSIXes
b98f989 
It also allows for passing -DOPENSSL_NO_LOCALE as a workaround
to ./Configure command.

Fixes #18233

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #18282)
openssl-machine pushed a commit that referenced this pull request May 13, 2022
@t8m
Include the e_os.h before string.h
cf91a2b 
Fixes #18244

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #18282)
openssl-machine pushed a commit that referenced this pull request May 13, 2022
@t8m
Move OPENSSL_strcasecmp() and related to o_str.c
71c17c3 
Otherwise the implementation is unnecessarily duplicated in legacy.so.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #18282)
openssl-machine pushed a commit that referenced this pull request May 13, 2022
@t8m
Avoid code duplication for locale initialization
26ccb0e 
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #18282)
openssl-machine pushed a commit that referenced this pull request May 13, 2022
@t8m
Add fallback in case of locale initialization failure
92d0501 
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #18282)
openssl-machine pushed a commit that referenced this pull request May 13, 2022
@t8m
Always try locale initialization from OPENSSL_strcasecmp
f505be9 
Fixes #18172

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #18282)
@rsbeckerca
Copy link
Contributor

This runs through our CI build/test cleanly on master, except for previously reported breakages #17537, #17816, and the new #18306 as of today.

@vszakats vszakats mentioned this pull request May 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulidale paulidale paulidale approved these changes

@rsbeckerca rsbeckerca rsbeckerca left review comments

@beldmit beldmit beldmit approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@t8m @beldmit @rsbeckerca @paulidale @openssl-machine