-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
crypto: build warning fixes #10142
crypto: build warning fixes #10142
Conversation
Resolve warning in Ubuntu Linux i386 build
Resolve conversion warning on Windows
BN_ULONG exponent_word = BN_get_word(rsa->e); | ||
BIO_printf(bio, "0x%lx", exponent_word); | ||
unsigned long exponent = // NOLINT(runtime/int) | ||
static_cast<unsigned long>(BN_get_word(rsa->e)); // NOLINT(runtime/int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct per se on 32 bits architectures where long
is 32 bits and BN_ULONG is a 64 bits unsigned long long
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix this tomorrow.
@@ -146,7 +149,8 @@ void ClientHelloParser::ParseExtension(const uint16_t type, | |||
ocsp_request_ = 1; | |||
break; | |||
case kTLSSessionTicket: | |||
tls_ticket_size_ = len; | |||
// TBD POSSIBLE DATA LOSS: | |||
tls_ticket_size_ = static_cast<uint16_t>(len & kTLSTicketSizeMask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@indutny Is there a reason tls_ticket_size_
is of type uint16_t instead of size_t?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how much bits it occupies in binary representation. I don't have any strong feelings about making it wider, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is what I gather from a quick grep: tls_ticket_size_
stores a ticket size and tls_ticket_
stores a pointer to some ticket information but they are only used to determine a hello.has_ticket_
boolean value. I start to wonder if we could replace the tls_ticket_
and tls_ticket_size_
members with a member like has_valid_tls_ticker_
to avoid keeping more state than necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In src/node_crypto.c
> @@ -1536,8 +1536,9 @@ static Local<Object> X509ToObject(Environment*
env, X509* cert) { String::kNormalString, mem->length)); (void)
BIO_reset(bio); - BN_ULONG exponent_word = BN_get_word(rsa->e); -
BIO_printf(bio, "0x%lx", exponent_word); + unsigned long exponent = //
NOLINT(runtime/int) + static_cast<unsigned long>(BN_get_word(rsa->e)); //
NOLINT(runtime/int)
This is not correct per se on 32 bits architectures where long is 32 bits
and BN_ULONG is a 64 bits unsigned long long.
I will look at this.
In src/node_crypto_clienthello.cc:
> @@ -146,7 +149,8 @@ void ClientHelloParser::ParseExtension(const
uint16_t type, ocsp_request_ = 1; break; case kTLSSessionTicket: -
tls_ticket_size_ = len; + // TBD POSSIBLE DATA LOSS: + tls_ticket_size_ =
static_cast<uint16_t>(len & kTLSTicketSizeMask);
@indutny Is there a reason tls_ticket_size_ is of type uint16_t instead
of size_t?
:+1: for fixing on my part.
@bnoorduis I will look at these and your comments on the other issues when
I get home tonight or maybe tomorrow morning. Thanks for the feedback!
|
Closing for now, I would like to deal with this later. |
Checklist
make -j8 test
(UNIX [macOS]; Linux i386 & amd64) andvcbuild test nosign
(.\vcbuild.bat nosign
then.\vcbuild.bat test nosign nobuild
as Administrator on Windows) PASS OKAdditional item
Affected core subsystem(s)
crypto
Description of change
NOTE: This was originally a part of PR #10139 (build, warning, header, and include fixes).
ADDITIONAL NOTE: This and other changes related to PR #10139 are marked
// TBD POSSIBLE DATA LOSS:
since I think we should examine these conversions at some point. I would be happy to take some or all of these out if necessary.