Avoid loading of a dynamic engine twice by bernd-edlinger · Pull Request #17073 · openssl/openssl

bernd-edlinger · 2021-11-19T11:05:54Z

Use the address of the bind function as a DYNAMIC_ID,
since the true name of the engine is not known
before the bind function returns,
but invoking the bind function before the engine
is unloaded results in memory corruption.

Fixes #17023

This is an alternative to #17055

Use the address of the bind function as a DYNAMIC_ID, since the true name of the engine is not known before the bind function returns, but invoking the bind function before the engine is unloaded results in memory corruption. Fixes openssl#17023

t8m · 2021-11-19T12:45:43Z

This looks much better than the alternative solution.

beldmit · 2021-11-19T13:01:29Z

I like this approach and kindly ask to add a test.

bernd-edlinger · 2021-11-19T13:06:36Z

okay, thanks for reminding me about the test case, one simple thing that was broken before this and gets fixed is
the memory leak I mentioned here:
#17025 (comment)
../util/shlib_wrap.sh ./openssl s_client -engine $PWD/../engines/ossltest.so -engine $PWD/../engines/ossltest.so
so it is a bit O/S dependent, though.

crypto/engine/eng_list.c

t8m

Yeah adding a test case would be nice.

beldmit · 2021-11-19T15:13:39Z

I'd suggest to withdraw an approval for now.

bernd-edlinger · 2021-11-19T15:45:07Z

Test case added, please re-confirm. Thanks!

t8m · 2021-11-19T16:32:52Z

Good for all branches, I suppose cherry-pick to 1.1.1 will require separate PR.

beldmit

LGTM. Nice work!

crypto/engine/eng_list.c

crypto/engine/eng_lib.c

openssl-machine · 2021-11-20T17:00:11Z

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.

bernd-edlinger · 2021-11-20T17:59:31Z

Hi @t8m @beldmit
I think the PR is now finalized.
Please re-confirm your approvals.

Thanks
Bernd.

petrovr · 2021-11-21T07:42:14Z

I'm still not convinced that exist defect that needs code correction.
Now I waste some time to test engines and result is #17023 (comment) .

eitkin-nvidia · 2021-11-21T11:05:12Z

I agree, and prefer this suggested fix over my approach in #17055. I think this is a good solution to the multi-load issue.

Thanks for taking the time to implement it on your side.

openssl-machine · 2021-11-23T00:00:20Z

This pull request is ready to merge

Use the address of the bind function as a DYNAMIC_ID, since the true name of the engine is not known before the bind function returns, but invoking the bind function before the engine is unloaded results in memory corruption. Fixes #17023 Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> (Merged from #17073) (cherry picked from commit e2571e0)

Use the address of the bind function as a DYNAMIC_ID, since the true name of the engine is not known before the bind function returns, but invoking the bind function before the engine is unloaded results in memory corruption. Fixes #17023 Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> (Merged from #17073)

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> (Merged from #17073)

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> (Merged from #17073) (cherry picked from commit 2595eef)

bernd-edlinger · 2021-11-23T05:13:25Z

Merged to all branches. Thanks!

Use the address of the bind function as a DYNAMIC_ID, since the true name of the engine is not known before the bind function returns, but invoking the bind function before the engine is unloaded results in memory corruption. Fixes #17023 Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> (Merged from #17073) (cherry picked from commit e2571e0)

bernd-edlinger added branch: master Applies to master branch branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) branch: 3.0 Applies to openssl-3.0 branch labels Nov 19, 2021

Avoid loading of a dynamic engine twice

4ce8760

Use the address of the bind function as a DYNAMIC_ID, since the true name of the engine is not known before the bind function returns, but invoking the bind function before the engine is unloaded results in memory corruption. Fixes openssl#17023

bernd-edlinger force-pushed the fix_duplicate_engine_loading branch from 1faf666 to 4ce8760 Compare November 19, 2021 11:15

bernd-edlinger mentioned this pull request Nov 19, 2021

Prevent dynamic loading of an already loaded engine #17055

Closed

t8m added the triaged: bug The issue/pr is/fixes a bug label Nov 19, 2021

t8m approved these changes Nov 19, 2021

View reviewed changes

t8m added the approval: done This pull request has the required number of approvals label Nov 19, 2021

bernd-edlinger commented Nov 19, 2021

View reviewed changes

crypto/engine/eng_list.c Outdated Show resolved Hide resolved

fixup! Avoid loading of a dynamic engine twice

b0e7850

t8m approved these changes Nov 19, 2021

View reviewed changes

bernd-edlinger added 2 commits November 19, 2021 16:38

Add a test case for duplicate engine loading

510fdb8

fixup! Add a test case for duplicate engine loading

758433a

fixup! Add a test case for duplicate engine loading

d7f5756

t8m approved these changes Nov 19, 2021

View reviewed changes

t8m removed the approval: done This pull request has the required number of approvals label Nov 19, 2021

t8m added the approval: done This pull request has the required number of approvals label Nov 19, 2021

beldmit approved these changes Nov 19, 2021

View reviewed changes

bernd-edlinger mentioned this pull request Nov 19, 2021

Add a test case for duplicate engine loading #17083

Closed

bernd-edlinger commented Nov 19, 2021

View reviewed changes

crypto/engine/eng_list.c Outdated Show resolved Hide resolved

bernd-edlinger added 2 commits November 19, 2021 23:04

fixup! Avoid loading of a dynamic engine twice

79b3184

fixup! Avoid loading of a dynamic engine twice

f58e325

bernd-edlinger commented Nov 20, 2021

View reviewed changes

crypto/engine/eng_list.c Outdated Show resolved Hide resolved

bernd-edlinger commented Nov 20, 2021

View reviewed changes

crypto/engine/eng_lib.c Show resolved Hide resolved

fixup! Avoid loading of a dynamic engine twice

983fb46

t8m approved these changes Nov 21, 2021

View reviewed changes

t8m added approval: done This pull request has the required number of approvals and removed approval: done This pull request has the required number of approvals labels Nov 21, 2021

openssl-machine removed the approval: done This pull request has the required number of approvals label Nov 23, 2021

openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Nov 23, 2021

openssl-machine pushed a commit that referenced this pull request Nov 23, 2021

Add a test case for duplicate engine loading

2595eef

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> (Merged from #17073)

openssl-machine pushed a commit that referenced this pull request Nov 23, 2021

Add a test case for duplicate engine loading

38e2957

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Dmitry Belyavskiy <[email protected]> (Merged from #17073) (cherry picked from commit 2595eef)

bernd-edlinger closed this Nov 23, 2021

tniessen mentioned this pull request Dec 15, 2021

deps: update to openssl-3.0.1+quic nodejs/node#41177

Closed

baentsch added a commit to open-quantum-safe/openssl that referenced this pull request Jun 23, 2022

Uh oh!

Comments

Conversation

bernd-edlinger commented Nov 19, 2021

Uh oh!

t8m commented Nov 19, 2021

Uh oh!

beldmit commented Nov 19, 2021

Uh oh!

bernd-edlinger commented Nov 19, 2021

Uh oh!

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

beldmit commented Nov 19, 2021

Uh oh!

bernd-edlinger commented Nov 19, 2021

Uh oh!

t8m commented Nov 19, 2021

Uh oh!

beldmit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openssl-machine commented Nov 20, 2021

Uh oh!

bernd-edlinger commented Nov 20, 2021

Uh oh!

petrovr commented Nov 21, 2021

Uh oh!

eitkin-nvidia commented Nov 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openssl-machine commented Nov 23, 2021

Uh oh!

bernd-edlinger commented Nov 23, 2021

Uh oh!

eitkin-nvidia commented Nov 21, 2021 •

edited

Loading