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

Provider common getters for ciphers and digest #9770

Closed
wants to merge 13 commits into from

Conversation

@paulidale
Copy link
Contributor

paulidale commented Sep 5, 2019

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

Fixes #9768
Fixes #9794

@paulidale paulidale force-pushed the paulidale:prov-common-digest branch from 9cd616d to aa64c1a Sep 5, 2019
@paulidale paulidale marked this pull request as ready for review Sep 5, 2019
@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 5, 2019

CMAC and GMAC went much faster than expected.

@paulidale paulidale added this to In progress in 3.0 New Core + FIPS via automation Sep 5, 2019
3.0 New Core + FIPS automation moved this from In progress to Needs review Sep 5, 2019
@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

Ah, so this is what you meant. Cool!

I have just one concern that occurred to me just now... I think we may have a bit of symbol name leakage.

$ ar t libcrypto.a | grep ^prov
@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 5, 2019

I'm not seeing the concerning output of the ar command.
I suspect I'm missing something obvious.

@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 5, 2019

Did you mean an nm of the .so?

These symbols are local, please ignore.

@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 5, 2019

Ah, so this is what you meant. Cool!

Yeah, I didn't explain myself well as usual :(

@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 5, 2019

I still don't understand what is meant by the ar command.
I'm also sure that I'm missing something really obvious...

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

I still don't understand what is meant by the ar command.

No wonder, I wrote that before coffee. This should make more sense:

$ nm -Pg libcrypto.a | grep ^prov
@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 5, 2019

Okay, I admit that there is some leakage.
I'm not convinced that this change has caused it.

It still needs to be addressed.

PS: Coffee is irrelevant :D

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

I'm not convinced that this change has caused it.

All the new utility functions you added end up in libcrypto and are a part of it. I'm sure there is more leakage that we simply haven't noticed before, well, now.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

I do note that there is a utility function remaining in providercommon.h with a name that starts with ossl_prov_. That's a precedent to follow.

@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 6, 2019

Okay, I'll switch to ossl_prov_ for the function names.

@paulidale paulidale force-pushed the paulidale:prov-common-digest branch from bd47352 to 4a253bb Sep 6, 2019
@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 6, 2019

Function names changed.

providers/common/provider_util.c Outdated Show resolved Hide resolved
providers/common/provider_util.c Outdated Show resolved Hide resolved
providers/common/provider_util.c Outdated Show resolved Hide resolved
providers/common/provider_util.c Outdated Show resolved Hide resolved
providers/common/provider_util.c Outdated Show resolved Hide resolved
providers/common/provider_util.c Outdated Show resolved Hide resolved
providers/common/provider_util.c Outdated Show resolved Hide resolved
@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 6, 2019

Travis is reporting aborts, among others when running the MAC tests with test/evp_test...

@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 6, 2019

It's failing with a segmentation fault inside EVP_DigestSignInit on the final SHA512 test case in evpmac.txt. Unfortunately, my debugger isn't operating at the moment.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 6, 2019

I can look at that abort some other day... won't have the time today, unfortunately

@paulidale paulidale force-pushed the paulidale:prov-common-digest branch from 5ba128e to 61a6c3b Sep 6, 2019
@paulidale paulidale force-pushed the paulidale:prov-common-digest branch from 1f6a63c to ebb4eaa Sep 7, 2019
@paulidale paulidale force-pushed the paulidale:prov-common-digest branch from ebb4eaa to df12a5b Sep 7, 2019
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
levitte pushed a commit that referenced this pull request Sep 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9770)
@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 7, 2019

Merged to master. Thanks.

@paulidale paulidale closed this Sep 7, 2019
3.0 New Core + FIPS automation moved this from Reviewer approved to Done Sep 7, 2019
@paulidale paulidale deleted the paulidale:prov-common-digest branch Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.