crypto/init.c: use destructor_key even as guard in OPENSSL_thread_stop. by dot-asm · Pull Request #6752 · openssl/openssl

dot-asm · 2018-07-20T11:36:22Z

Problem was that Windows threads that were terminating before libcrypto
was initialized were referencing uninitialized or possibly even
unrelated thread local storage index.

Request covers even couple of other issues related to dllmain. This is alternative to #6627 and #6711.

1.0.2 and 1.1.0 labels are speculative in sense that not everything should be back-ported and those commits that should be won't necessarily cherry-pick.

Problem was that Windows threads that were terminating before libcrypto was initialized were referencing uninitialized or possibly even unrelated thread local storage index.

kroeckx · 2018-07-21T00:46:20Z

crypto/cryptlib.c

+    else if (ossl_isxdigit(ossl_tolower(c)))
+        return c - 'a' + 10;
+
+    return 16;


It's largest possible base value, that will make caller terminate to the loop. Added commentary.

kroeckx · 2018-07-22T02:34:47Z

crypto/init.c

-static void ossl_init_thread_stop(struct thread_local_inits_st *locals);
+/*
+ * Since per-thread-specific-data destructors are not universally
+ * available, i.e. not on *cough*-dows, only below CRYPTO_THREAD_LOCAL


Can you write that as Windows?

kroeckx · 2018-07-22T02:37:08Z

crypto/cryptlib.c

+ * agnostic" means that they work with either wide or 8-bit characters,
+ * exploiting the fact that first 127 characters can be simply casted
+ * between the sets, while the rest would be simply rejected by ossl_is*
+ * subroutines.


I'm not sure how this is supposed to work. You get a char *. Every 2 characters will be a '\0' if it's UTF-16

I must have somehow missed the ossl_char

How about renaming ossl_char to variant_char? Hopefully it would be harder to miss then...

kroeckx · 2018-07-22T02:43:08Z

crypto/cryptlib.c

+    static WCHAR value[48];
+    DWORD len = GetEnvironmentVariableW(L"OPENSSL_ia32cap", value, 48);
+
+    return (len > 0 && len <= 48) ? value : NULL;


As far as I understand, you can never get 48 back. But I would expect the check to be < 48.

Oh! I thought that it returns value that accounts even for trailing null. But it does so only when buffer is not large enough. Thanks for spotting this!

kroeckx · 2018-07-22T02:43:44Z

crypto/cryptlib.c

+ */
+#  ifdef _WIN32
+typedef WCHAR ossl_char;
+static ossl_char *ossl_getenv(const char *name)


The name parameter is unused ...

Correct. Basically there are two alternatives to this. a) Make conversion from char to WCHAR with MutliByteToWideChar in this subroutine. b) Arrange preprocessor directives so that "OPENSSL_ia32cap" literal is passed to ossl_getenv as L"OPENSSL_ia32cap" on Windows. Chosen approach (of ignoring the argument and resorting to hard-coded L-literal that works ~~with~~ specifically here) is least-typing one. Would a commentary be sufficient of should I switch to alternative?

A comment is good for me.

kroeckx · 2018-07-22T03:00:57Z

crypto/init.c


-    CRYPTO_THREAD_cleanup_local(&threadstopkey);
+    key = destructor_key;
+    destructor_key = (CRYPTO_THREAD_LOCAL)-1;


Why does it get set to -1?

To denote the fact that it's no longer valid. Since libcrypto leaks descriptor to itself (not fan of it by the way), it won't go away from memory, so that Windows will keep invoking DLL_THREAD_DETACH and OPENSSL_thread_stop even after OPENSSL_cleanup. So that destructor_key has to go back to "impossible" value.

I've mentioned post-cleanup case in commentary to destructor_key.

…e squashed].

dot-asm · 2018-07-22T08:18:19Z

Feedback is addressed.

dot-asm · 2018-07-22T08:25:44Z

Spotted bug in todigit!

…to be squashed]

dot-asm · 2018-07-22T09:47:16Z

I've spotted typo in commentary, MSCVRT vs. MSVCRT, which fixed in first post-approval commit. Since it's commentary-only change I claim that it doesn't invalidate approval status (unless somebody claims otherwise). But on top of that I did ossl_char -> variant_char rename, which certainly invalidates approval status. However! I leave lack of re-approval as option. In sense that [if] request is not re-approved with variant_char, then I intend to merge commits up to this last one.

kroeckx · 2018-07-22T09:54:50Z

I don't really care how you name that type, so consider this +1.
But I wonder why you call it "variant"

dot-asm · 2018-07-22T10:09:52Z

But I wonder why you call it "variant"

There might be better options. "variant" can be used as adjective and does describe something that can change depending on situation. In this case depending on whether or not code is compiled for Windows.

Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from #6752)

Problem was that Windows threads that were terminating before libcrypto was initialized were referencing uninitialized or possibly even unrelated thread local storage index. Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from #6752)

Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from #6752) (cherry picked from commit 9e4a1c3)

dot-asm · 2018-07-25T14:55:41Z

9e4a1c3 was the only that cherry-picked to 1.1.0. So that will be branch-specific requests...

Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from openssl#6752) (cherry picked from commit b86d57b) Resolved conflicts: crypto/cryptlib.c

Problem was that Windows threads that were terminating before libcrypto was initialized were referencing uninitialized or possibly even unrelated thread local storage index. Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from openssl#6752) (cherry picked from commit 80ae728) Resolved conflicts: crypto/init.c

Andy Polyakov added 4 commits July 20, 2018 13:30

crypto/cryptlib.c: make OPENSS_cpuid_setup safe to use as constructor.

f01757f

crypto/cryptlib.c: resolve possible race in OPENSSL_isservice.

39aaf16

crypto/dllmain.c: remove unused OPENSSL_NONPIC_relocated variable.

bb45f14

crypto/init.c: use destructor_key even as guard in OPENSSL_thread_stop.

debd660

Problem was that Windows threads that were terminating before libcrypto was initialized were referencing uninitialized or possibly even unrelated thread local storage index.

dot-asm added branch: master Applies to master branch branch: 1.0.2 Applies to OpenSSL_1_0_2-stable branch (EOL) 1.1.0 branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) labels Jul 20, 2018

mattcaswell added this to the Assessed milestone Jul 20, 2018

kroeckx reviewed Jul 21, 2018

View reviewed changes

crypto/cryptlib.c: add commentary [to be squashed] [skip ci]

6b4b26f

kroeckx reviewed Jul 22, 2018

View reviewed changes

Andy Polyakov added 2 commits July 22, 2018 10:14

crypto/cryptlib.c: fix error in Windows code and add commentary [to b…

f19337e

…e squashed].

crypto/init.c: refine commentary [to be squashed].

70b452a

crypto/cryptlib.c: refine commentary [to be squashed] [skip ci]

4bd9333

crypto/cryptlib.c: fix bug in todigit [to be squashed]

a672f8a

kroeckx approved these changes Jul 22, 2018

View reviewed changes

kroeckx added the approval: done This pull request has the required number of approvals label Jul 22, 2018

Andy Polyakov added 2 commits July 22, 2018 11:39

crypto/cryptlib.c: fix typo in commentary [to be squashed]

123c726

crypto/cryptlib.c: ossl_char -> variant_char for better readability […

84dfd92

…to be squashed]

levitte pushed a commit that referenced this pull request Jul 25, 2018

crypto/cryptlib.c: make OPENSS_cpuid_setup safe to use as constructor.

b86d57b

Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from #6752)

levitte pushed a commit that referenced this pull request Jul 25, 2018

crypto/cryptlib.c: resolve possible race in OPENSSL_isservice.

9e4a1c3

Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from #6752)

levitte pushed a commit that referenced this pull request Jul 25, 2018

crypto/dllmain.c: remove unused OPENSSL_NONPIC_relocated variable.

ceb8e32

Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from #6752)

levitte pushed a commit that referenced this pull request Jul 25, 2018

crypto/cryptlib.c: resolve possible race in OPENSSL_isservice.

1de6818

Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from #6752) (cherry picked from commit 9e4a1c3)

dot-asm closed this Jul 25, 2018

This was referenced Jul 27, 2018

crypto/init.c: use destructor_key even as guard in OPENSSL_thread_stop. #6799

Closed

DllMain should allocate TLS on DLL_PROCESS_ATTACH #6627

Closed

Uh oh!

Conversation

dot-asm commented Jul 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dot-asm Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroeckx Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dot-asm Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

dot-asm commented Jul 22, 2018

Uh oh!

dot-asm commented Jul 22, 2018

Uh oh!

dot-asm commented Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kroeckx commented Jul 22, 2018

Uh oh!

dot-asm commented Jul 22, 2018

Uh oh!

dot-asm commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dot-asm Jul 21, 2018 •

edited

Loading

kroeckx Jul 22, 2018 •

edited

Loading

dot-asm Jul 22, 2018 •

edited

Loading

dot-asm commented Jul 22, 2018 •

edited

Loading