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

Windows DLL stays locked when linked against OpenSSL 1.1.1j #16024

Closed
hbeni opened this issue Jul 8, 2021 · 8 comments
Closed

Windows DLL stays locked when linked against OpenSSL 1.1.1j #16024

hbeni opened this issue Jul 8, 2021 · 8 comments
Labels
branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch triaged: question The issue contains a question

Comments

@hbeni
Copy link

hbeni commented Jul 8, 2021

Hello there,

I have a plugin for mumble in the works, which faciliates an updater. Mumble can load and unload the plugins. They are just DLLs exporting a bunch of functions. Mumble also has an updater, that calls a plugin function to get the latest download URL. It then fetches the new plugin, unloads the current one if loaded, and then ⚠️ deletes the old plugin dll to overwrite it with the downloaded version.

This is the point where I get problems:
As soon as I compile against OpenSSL (1.1.1j is in effect here) the DLL gets locked by mumble and could not be free'd entirely and thus cannot be deleted by mumble.

  • We already tracked this down to be specifically related to cpp-httplib and/or OpenSSL
  • We ruled out it'ts a mumble or generic plugin issue
  • I think the Problem is something in OpenSSL (or maybe the way cpp-httplib instanciates/uses it?)
  • Just linking OpenSSL does not trigger the issue, it is only there if #define CPPHTTPLIB_OPENSSL_SUPPORT 1 to let cpp-httplib uses it

Details:

@hbeni hbeni added the issue: bug report The issue was opened to report a bug label Jul 8, 2021
@paulidale paulidale added triaged: question The issue contains a question branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch and removed issue: bug report The issue was opened to report a bug labels Jul 8, 2021
@hbeni
Copy link
Author

hbeni commented Jul 8, 2021

Hi, I just got new insights.

I wondered if it might have to do with a specific OpenSSL version, and indeed it is the case.

Versions 1.1.1a and 1.1.1b are working fine, but from 1.1.1c onward the issue described above is present.
(I also suggest tagging this as bug again for that reason)

Now the question is - which of the 23250 136 commits does cause this...
(edit) seems to be less than that - bisecting now.

@hbeni
Copy link
Author

hbeni commented Jul 8, 2021

The bisect is done:

fc4c034 is the first bad commit
commit fc4c034
Author: Guido Vranken guidovranken@gmail.com
Date: Mon Apr 22 14:11:12 2019 +0200

Enforce a strict output length check in CRYPTO_ccm128_tag

Return error if the output tag buffer size doesn't match
the tag size exactly. This prevents the caller from
using that portion of the tag buffer that remains
uninitialized after an otherwise succesfull call to
CRYPTO_ccm128_tag.

Bug found by OSS-Fuzz.

Fix suggested by Kurt Roeckx.

Signed-off-by: Guido Vranken <guidovranken@gmail.com>

Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/8810)

(cherry picked from commit 514c9da48b860153079748b0d588cd42191f0b6a)

crypto/modes/ccm128.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

@hbeni
Copy link
Author

hbeni commented Jul 8, 2021

That doesn't make so much sense.
For reference, this is what I recorded during the bisect:

2341db5 bad
66790d7 bad
7216e9a bad
fc4c034 bad
57aac8b bad
282360e good
7a3c4b3 good

(edit: reordered chronocally)

@hbeni
Copy link
Author

hbeni commented Jul 8, 2021

Ok, I just tried to revert fc4c034 and it is still not good.
Will dig deeper and report back.

@hbeni
Copy link
Author

hbeni commented Jul 9, 2021

Today I recompiled and tested 1.1.1a up until commit 97ace46 (1.1.1c) and that worked.
Neglect this; this was mistesting by me. Obviously you have to make sure to install the same dll. But for testing i had git commit hashes in the name, so this obviously just installed a new plugin with different name in terms of mumble.

Currently retesting.

edit:
Now faulty range are the ten commits between:

fgcom-mumble.openssl_32a775df9b.dll	bad
fgcom-mumble.openssl_d7af859880.dll	good

@hbeni
Copy link
Author

hbeni commented Jul 9, 2021

OK, I think I found it, and this tme it does make sense (probably):
It's part of OpenSSL_1_1_1c

5fba3afad0      bad
0c45bd8dae	good

5fba3af is the first bad commit
commit 5fba3af
Author: Richard Levitte levitte@openssl.org
Date: Mon Apr 1 06:40:33 2019 +0200

Rework DSO API conditions and configuration option

'no-dso' is meaningless, as it doesn't get any macro defined.
Therefore, we remove all checks of OPENSSL_NO_DSO.  However, there may
be some odd platforms with no DSO scheme.  For those, we generate the
internal macro DSO_NONE aand use it.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/8622)

Configure | 23 ++++++++++-------------
INSTALL | 3 ---
crypto/dso/dso_openssl.c | 2 +-
crypto/include/internal/dso_conf.h.in | 5 +++--
crypto/init.c | 10 ++++------
include/internal/dsoerr.h | 7 ++-----
6 files changed, 20 insertions(+), 30 deletions(-)

I tried to revert 5fba3af on tag OpenSSL_1_1_1c, but this yields conflicts I don't understand.

Hopefully this will give you enough information to figure out what's going on.

@t8m
Copy link
Member

t8m commented Jul 9, 2021

You can try configuring and building with no-pinshared.

@hbeni
Copy link
Author

hbeni commented Jul 9, 2021

You can try configuring and building with no-pinshared.

That fixed it! Now it works with 1.1.1k :D 🥳

@hbeni hbeni closed this as completed Jul 9, 2021
hbeni added a commit to hbeni/fgcom-mumble that referenced this issue Jul 9, 2021
…cked DLL

Issue was that OpenSSL introduced some stuff breaking this.
Solution from OpenSSL-Team is to compile with `no-pinshared`
(ref: openssl/openssl#16024 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch triaged: question The issue contains a question
Projects
None yet
Development

No branches or pull requests

3 participants