Add generic EVP method fetcher by levitte · Pull Request #8341 · openssl/openssl

levitte · 2019-02-26T09:13:47Z

This is an interface between Core dispatch table fetching and
EVP_{method}_fetch(). All that's needed from the diverse method
fetchers are the functions to create a method structure from a
dispatch table, a function that ups the method reference counter and a
function to free the method (in case of failure).

This routine is internal to the EVP API andis therefore only made
accessible within crypto/evp, by including evp_locl.h

Checklist

documentation is added or updated
tests are added or updated

levitte · 2019-02-26T09:15:06Z

To build, this needs #8286, #8287 and #8340. It's here to show how internal API specific methods can get created from provider dispatch tables through the core fetcher from #8340.

paulidale · 2019-02-28T03:02:19Z

crypto/evp/evp_fetch.c

Would it make sense to include this from evp_locl.h if it is needed there?

crypto/evp/evp_fetch.c

paulidale · 2019-03-04T21:54:33Z

CI errors are relevant.

levitte · 2019-03-04T22:59:07Z

@paulidale, in #8341 (comment), I mention that this needs #8286, #8287 and #8340. Only #8286 has yet made it into master.

paulidale · 2019-03-04T23:12:52Z

Okay, I'll look at them too.

doc/internal/man3/evp_generic_fetch.pod

crypto/evp/evp_fetch.c

levitte · 2019-03-11T19:46:49Z

I've rebased this on top of a fresh master. I'll go through any remaining comments tomorrow

crypto/evp/evp_fetch.c

mattcaswell · 2019-03-12T15:41:23Z

crypto/evp/evp_fetch.c

Do we need some locking here?

No, not here. Locking for this should be part of the object database code, possibly through a function that anyone can call.

That should be done in a separate PR, and locking calls should be added in all places where things are added to the object database if necessary, not just here.

(and for anyone listening, PR welcome!)

We should raise an issue for this

crypto/evp/evp_fetch.c

levitte · 2019-03-12T19:17:36Z

Please note, btw, that this won't work without #8340...

levitte · 2019-03-12T19:28:05Z

#8340 is now merged, and this has been rebased on top.

mattcaswell · 2019-03-13T09:36:28Z

Travis errors look relevant:

crypto/evp/evp_fetch.c: In function 'evp_generic_fetch':
crypto/evp/evp_fetch.c:168:9: error: initializer element is not computable at load time [-Werror]
         };
         ^
cc1: all warnings being treated as errors
make[1]: *** [crypto/evp/libcrypto-shlib-evp_fetch.o] Error 1
make[1]: Leaving directory `/home/travis/build/openssl/openssl'
make: *** [build_programs] Error 2

levitte · 2019-03-13T10:22:40Z

Saw it, and discovered a small thing missing in #8340. Corrected with 837f046

levitte · 2019-03-13T12:52:46Z

CIs are happy now

mattcaswell · 2019-03-14T12:00:01Z

crypto/evp/evp_fetch.c

We should raise an issue for this

doc/internal/man3/evp_generic_fetch.pod

levitte · 2019-03-14T21:06:10Z

The work done today was to correct some refcounting that wasn't quite right.

All relevant OSSL_METHOD_CONSTRUCT_METHOD callbacks got the callback data passed to them, except 'destruct'. There's no reason why it shouldn't get that pointer passed, so we make a small adjustment.

Fully assume that the method constructors use reference counting. Otherwise, we may leak memory, or loose track and do a double free.

This is an interface between Core dispatch table fetching and EVP_{method}_fetch(). All that's needed from the diverse method fetchers are the functions to create a method structure from a dispatch table, a function that ups the method reference counter and a function to free the method (in case of failure). This routine is internal to the EVP API andis therefore only made accessible within crypto/evp, by including evp_locl.h

mattcaswell

Approved assuming the minor typo is fixed. I also think an issue should be raised for the OBJ locking stuff before it gets forgotten about.

mattcaswell · 2019-03-18T10:44:32Z

crypto/core_fetch.c

+         * Note regarding putting the method in stores:
+         *
+         * we don't need to care if it actually got in or not here.
+         * If it didn't get it, it will simply not be available when


s/get it/get in/

levitte · 2019-03-18T13:27:54Z

Merged.

Thanks for the reviews!

c13d2ab Add generic EVP method fetcher
a383083 Replumbing: better reference counter control in ossl_method_construct()
7bb19a0 Replumbing: pass callback data to the algo destructor too

All relevant OSSL_METHOD_CONSTRUCT_METHOD callbacks got the callback data passed to them, except 'destruct'. There's no reason why it shouldn't get that pointer passed, so we make a small adjustment. Reviewed-by: Matt Caswell <[email protected]> (Merged from #8341)

Fully assume that the method constructors use reference counting. Otherwise, we may leak memory, or loose track and do a double free. Reviewed-by: Matt Caswell <[email protected]> (Merged from #8341)

This is an interface between Core dispatch table fetching and EVP_{method}_fetch(). All that's needed from the diverse method fetchers are the functions to create a method structure from a dispatch table, a function that ups the method reference counter and a function to free the method (in case of failure). This routine is internal to the EVP API andis therefore only made accessible within crypto/evp, by including evp_locl.h Reviewed-by: Matt Caswell <[email protected]> (Merged from #8341)

levitte added the branch: master Applies to master branch label Feb 26, 2019

levitte force-pushed the 300-evpfetch branch 3 times, most recently from 00d3174 to ae3784f Compare February 27, 2019 21:33

paulidale reviewed Feb 28, 2019

View reviewed changes

levitte force-pushed the 300-evpfetch branch from 9911a5c to 5af5a9a Compare March 5, 2019 08:35

slontis reviewed Mar 6, 2019

View reviewed changes

doc/internal/man3/evp_generic_fetch.pod Outdated Show resolved Hide resolved

slontis reviewed Mar 6, 2019

View reviewed changes

doc/internal/man3/evp_generic_fetch.pod Outdated Show resolved Hide resolved

slontis reviewed Mar 6, 2019

View reviewed changes

doc/internal/man3/evp_generic_fetch.pod Outdated Show resolved Hide resolved

slontis reviewed Mar 6, 2019

View reviewed changes

crypto/evp/evp_fetch.c Outdated Show resolved Hide resolved

levitte force-pushed the 300-evpfetch branch 2 times, most recently from 2996ee6 to 35774f1 Compare March 11, 2019 19:46

mattcaswell requested changes Mar 12, 2019

View reviewed changes

mattcaswell reviewed Mar 12, 2019

View reviewed changes

crypto/evp/evp_fetch.c Outdated Show resolved Hide resolved

levitte force-pushed the 300-evpfetch branch from 09a3e2a to d7ad474 Compare March 12, 2019 19:27

levitte force-pushed the 300-evpfetch branch from d7ad474 to e77f4ca Compare March 13, 2019 10:21

mattcaswell requested changes Mar 14, 2019

View reviewed changes

levitte force-pushed the 300-evpfetch branch from e77f4ca to c9a8592 Compare March 14, 2019 21:00

Replumbing: pass callback data to the algo destructor too

7c14d67

All relevant OSSL_METHOD_CONSTRUCT_METHOD callbacks got the callback data passed to them, except 'destruct'. There's no reason why it shouldn't get that pointer passed, so we make a small adjustment.

levitte added 7 commits March 17, 2019 19:49

Replumbing: better reference counter control in ossl_method_construct()

ea2a020

Fully assume that the method constructors use reference counting. Otherwise, we may leak memory, or loose track and do a double free.

fixup! Add generic EVP method fetcher

397425d

fixup! Add generic EVP method fetcher

f2cebd7

fixup! Add generic EVP method fetcher

027b04a

fixup! Add generic EVP method fetcher

4ac5ec4

fixup! Add generic EVP method fetcher

624424b

levitte force-pushed the 300-evpfetch branch from 434ea56 to 624424b Compare March 17, 2019 18:50

mattcaswell approved these changes Mar 18, 2019

View reviewed changes

mattcaswell added the approval: done This pull request has the required number of approvals label Mar 18, 2019

mattcaswell mentioned this pull request Mar 18, 2019

Add a default provider and implement SHA256 in it #8513

Closed

levitte closed this Mar 18, 2019

Uh oh!

Conversation

levitte commented Feb 26, 2019

Checklist

Uh oh!

levitte commented Feb 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulidale commented Mar 4, 2019

Uh oh!

levitte commented Mar 4, 2019

Uh oh!

paulidale commented Mar 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

levitte commented Mar 11, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

levitte commented Mar 12, 2019

Uh oh!

levitte commented Mar 12, 2019

Uh oh!

mattcaswell commented Mar 13, 2019

Uh oh!

levitte commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Mar 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

levitte commented Mar 14, 2019

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte commented Mar 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

levitte commented Mar 13, 2019 •

edited

Loading