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.

Sign up for GitHub

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

Jump to bottom

src: improve SSL version extraction logic #23050

Closed
wants to merge 2 commits into from
Closed

src: improve SSL version extraction logic #23050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa8470e
src: improve SSL version extraction logic
gireeshpunathil Sep 24, 2018
0ad3a8d
fixup: use constexpr to optimize
gireeshpunathil Sep 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 5 additions & 38 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,24 +237,7 @@ class NodeTraceStateObserver :
trace_process->SetString("napi", node_napi_version);

#if HAVE_OPENSSL
// Stupid code to slice out the version string.
{ // NOLINT(whitespace/braces)
size_t i, j, k;
int c;
for (i = j = 0, k = sizeof(OPENSSL_VERSION_TEXT) - 1; i < k; ++i) {
c = OPENSSL_VERSION_TEXT[i];
if ('0' <= c && c <= '9') {
for (j = i + 1; j < k; ++j) {
c = OPENSSL_VERSION_TEXT[j];
if (c == ' ')
break;
}
break;
}
}
trace_process->SetString("openssl",
std::string(&OPENSSL_VERSION_TEXT[i], j - i));
}
trace_process->SetString("openssl", crypto::GetOpenSSLVersion());
#endif
trace_process->EndDictionary();

Expand Down Expand Up @@ -1764,26 +1747,10 @@ void SetupProcessObject(Environment* env,
FIXED_ONE_BYTE_STRING(env->isolate(), node_napi_version));

#if HAVE_OPENSSL
// Stupid code to slice out the version string.
{ // NOLINT(whitespace/braces)
size_t i, j, k;
int c;
for (i = j = 0, k = sizeof(OPENSSL_VERSION_TEXT) - 1; i < k; ++i) {
c = OPENSSL_VERSION_TEXT[i];
if ('0' <= c && c <= '9') {
for (j = i + 1; j < k; ++j) {
c = OPENSSL_VERSION_TEXT[j];
if (c == ' ')
break;
}
break;
}
}
READONLY_PROPERTY(
versions,
"openssl",
OneByteString(env->isolate(), &OPENSSL_VERSION_TEXT[i], j - i));
}
READONLY_PROPERTY(
versions,
"openssl",
OneByteString(env->isolate(), crypto::GetOpenSSLVersion().c_str()));
#endif

// process.arch
Expand Down
15 changes: 15 additions & 0 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5725,6 +5725,21 @@ void Initialize(Local<Object> target,
#endif // OPENSSL_NO_SCRYPT
}

constexpr int search(const char* s, int n, int c) {
return *s == c ? n : search(s + 1, n + 1, c);
}

std::string GetOpenSSLVersion() {
// sample openssl version string format
// for reference: "OpenSSL 1.1.0i 14 Aug 2018"
char buf[128];
const int start = search(OPENSSL_VERSION_TEXT, 0, ' ') + 1;
const int end = search(OPENSSL_VERSION_TEXT + start, start, ' ') + 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I posted a mildly incorrect version of search() (even though it looks like it worked out okay; amazing, computers usually aren't that forgiving.)

It should look like this:

constexpr int search(const char* s, int n, int c) {
  return s[n] == c ? n : search(s, n + 1, c);  // note: s isn't incremented
}

And the search for end should therefore be this:

const int end = search(OPENSSL_VERSION_TEXT, start, ' ') - 1; // note: -1 to exclude the blank

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you meant:
const int end = search(ssl, start, ' ') + 1;
instead of:
const int end = search(ssl, start, ' ') - 1; ?

else I get truncated output: for example when expecting 1.1.0i-fips I get 1.1.0i-fi

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this:

constexpr int SSLVersion_Helper2(const int s) {
    return OPENSSL_VERSION_TEXT[s + 7] == ' ' ? s + 7 : s + 12;
}

constexpr const int SSLVersion_Helper()
{
  // option 1 "OpenSSL 1.1.0i  14 Aug 2018"
  // option 2 "OpenSSL 1.1.0i-fips  14 Aug 2018"
  // 7 = strlen("OpenSSL");
  static_assert(OPENSSL_VERSION_TEXT[7] == ' ',
                OPENSSL_VERSION_TEXT " + 7 isn't a ' ' char");
  // 7 = strlen("OpenSSL 1.1.0i");
  // 12 = strlen("OpenSSL 1.1.0i-fips");
  static_assert(OPENSSL_VERSION_TEXT[SSLVersion_Helper2(7)] == ' ',
                OPENSSL_VERSION_TEXT " + 14 or 19 isn't a ' ' char");
  return SSLVersion_Helper2(7);
}

const std::string kSSLVersionString{OPENSSL_VERSION_TEXT + 7, 
                                    OPENSSL_VERSION_TEXT + SSLVersion_Helper()};

As K.I.S.S. as I could while it still compiles on GCC4.9

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @refack . I am seeing it as compiled into a regular function, with no static awareness of the const string or const int passed to it and leveraged by the compiler. Is it only my gcc? can you please check?

(gdb) x/16i SSLVersion_Helper2
   0x400eb8 <_Z18SSLVersion_Helper2i>:	push   rbp
   0x400eb9 <_Z18SSLVersion_Helper2i+1>:	mov    rbp,rsp
   0x400ebc <_Z18SSLVersion_Helper2i+4>:	mov    DWORD PTR [rbp-0x4],edi
   0x400ebf <_Z18SSLVersion_Helper2i+7>:	mov    eax,DWORD PTR [rbp-0x4]
   0x400ec2 <_Z18SSLVersion_Helper2i+10>:	add    eax,0x7
   0x400ec5 <_Z18SSLVersion_Helper2i+13>:	cdqe   
   0x400ec7 <_Z18SSLVersion_Helper2i+15>:	movzx  eax,BYTE PTR [rax+0x401228]
   0x400ece <_Z18SSLVersion_Helper2i+22>:	cmp    al,0x20
   0x400ed0 <_Z18SSLVersion_Helper2i+24>:	jne    0x400eda <_Z18SSLVersion_Helper2i+34>
   0x400ed2 <_Z18SSLVersion_Helper2i+26>:	mov    eax,DWORD PTR [rbp-0x4]
   0x400ed5 <_Z18SSLVersion_Helper2i+29>:	add    eax,0x7
   0x400ed8 <_Z18SSLVersion_Helper2i+32>:	jmp    0x400ee0 <_Z18SSLVersion_Helper2i+40>
   0x400eda <_Z18SSLVersion_Helper2i+34>:	mov    eax,DWORD PTR [rbp-0x4]
   0x400edd <_Z18SSLVersion_Helper2i+37>:	add    eax,0xc
   0x400ee0 <_Z18SSLVersion_Helper2i+40>:	pop    rbp
   0x400ee1 <_Z18SSLVersion_Helper2i+41>:	ret    
(gdb)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in other words, where do we see the benefit of the constexpr optimizations that was the theme of this PR for sometime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more for the academics: https://godbolt.org/z/qWyDbk show it being compile time only (with -O2)

const int len = end - start;
snprintf(buf, len, "%.*s\n", len, &OPENSSL_VERSION_TEXT[start]);
Copy link
Member

@bnoordhuis bnoordhuis Oct 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second arg should have been sizeof(buf), not len. I'll open a pull request. (edit: #23622)

return std::string(buf);
}

} // namespace crypto
} // namespace node

Expand Down
1 change: 1 addition & 0 deletions src/node_crypto.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ extern int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx);
extern void UseExtraCaCerts(const std::string& file);

void InitCryptoOnce();
std::string GetOpenSSLVersion();

class SecureContext : public BaseObject {
public:
Expand Down