-
Notifications
You must be signed in to change notification settings - Fork 30k
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
n-api: fix warning about size_t compare with int #15508
Conversation
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.
LGTM
src/node_api.cc
Outdated
@@ -920,13 +920,13 @@ NAPI_NO_RETURN void napi_fatal_error(const char* location, | |||
size_t message_len) { | |||
char* location_string = const_cast<char*>(location); | |||
char* message_string = const_cast<char*>(message); | |||
if (location_len != -1) { | |||
if (static_cast<int>(location_len) != -1) { |
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.
size_t might allow for larger values than signed ints and we're wrongly casting 2^n-1
to -1
(where n
is how big size_t is). If we want to allow -1
here, why don't we change the parameter type? Or could we use 0
instead of -1
to signal that we don't want to set the message_string
?
src/node_api.cc
Outdated
@@ -920,13 +920,13 @@ NAPI_NO_RETURN void napi_fatal_error(const char* location, | |||
size_t message_len) { | |||
char* location_string = const_cast<char*>(location); | |||
char* message_string = const_cast<char*>(message); | |||
if (location_len != -1) { | |||
if (static_cast<int>(location_len) != -1) { | |||
location_string = reinterpret_cast<char*>( | |||
malloc(location_len * sizeof(char) + 1)); |
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.
Aside: sizeof(char)
is 1 by definition and reinterpret_cast
is overkill here, static_cast
would have sufficed.
What's more, I don't understand why this code makes copies. This function does not return. It seems like completely unneeded complexity.
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.
The reason for doing copy here is to account for strings that are not null terminated.
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.
Okay, but the other two points still stand. As well, you don't handle nullptr return values. If you use node::Malloc()
, that's taken care of.
Or use std::string
, that will also stop Coverity from complaining about memory leaks.
@sampsongao can you look at the types that are used by the similar calls in V8 that already had the -1 as the option to indicate null terminated ? We were already passing through -1 to those calls and maybe we need to use the same type here. |
Talking with Sampson, in many cases we just pass the -1 on to an v8 call so I don't think we'd want to switch to using 0 instead as then we'd need code to covert. It may make sense to just change the type to int. I think we discussed size_t versus int before but we may not have factored in that -1 is not valid for size_t |
It's valid (it wraps around to
What about other engines? Abstracting away such details is the point of n-api, isn't it? |
For some APIs that take a string length, for example Should we consider |
What's wrong with passing the size explicitly? The only reason I can think of is programmer ergonomics but passing |
We'd discussed the options in the past and agreed to support the -1 shortcut in the way that V8 does. The latest PR was just to make it consistent across the board. I think we should fix up the type problem and then discuss if we should remove the shortcut completely in a later PR. |
That would be fine with me also. In most cases the C++ wrapper would make that automatic anyway. But if we do that we should probably still detect when -1 is passed in, because the compiler usually won't complain and string APIs in many libraries do support it, so people may assume it works with N-API. Or we could document that |
SIZE_MAX would be fine with me as long as that works in the cases were we pass it on to V8 and its sounds like that will be the case. |
We discussed in N-API meeting today. Here is what we'd like to do
|
Lets update this PR to do 1-3 and I will submit a separate PR to update the doc. |
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.
LGTM
CI run: https://ci.nodejs.org/job/node-test-pull-request/10192/ @bnoordhuis, @fhinkel can you take a another look, we'd like this to make it into 8.6 which is going out next tuesday. |
src/node_api.cc
Outdated
@@ -120,13 +120,14 @@ struct napi_env__ { | |||
#define CHECK_NEW_FROM_UTF8_LEN(env, result, str, len) \ | |||
do { \ | |||
auto str_maybe = v8::String::NewFromUtf8( \ | |||
(env)->isolate, (str), v8::NewStringType::kInternalized, (len)); \ | |||
(env)->isolate, (str), v8::NewStringType::kInternalized, \ | |||
static_cast<int>(len)); \ |
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.
It would be nice to have a static_assert(static_cast<int>(NAPI_AUTO_LENGTH) == -1)
somewhere
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.
@addaleax that is a good idea.
Do you think it would make sense to add it just after
namespace {
namespace v8impl {
in node_api.cc
along with a comment indicating the v8 implementation of n-api depends on it being true ?
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.
@mhdawson I like to put that near where it’s actually used (i.e. around here) but feel free to put it wherever you like :)
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 was thinking you had somewhere else in mind, I'm good with it going into the macro itself
centos failures are #15505 |
Windows fanned failure was unrelated created - #15558 |
Arm failure was an infra issue, not test related. So overall CI looks good. |
src/node_api.cc
Outdated
std::string location_string; | ||
std::string message_string; | ||
|
||
if (static_cast<int>(location_len) != NAPI_AUTO_LENGTH) { |
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 don't think that size_t
always fits into an int
. Why do we need to cast here? (Feel free to land once this is addressed if I don't get around to change my review to approve
in time.)
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 agree it should not be needed in this check. In places where it is passed on to v8 then it is needed.
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.
@mhdawson No, the issue isn’t that it’s in the wrong place – it’s that a downcast is a real bug here, because it truncates location_len
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.
@addaleax, to clarify we know that size_t does not fit into an int, so where its going to be passed to v8 a cast is required to avoid warnings. In this case we should just be comparing NAPI_AUTO_LENGTH directly against location_len.
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.
@mhdawson Yeah, but we can’t just downcast without any checks in this case. Imagine a platform that has 32-bit int
, and 64-bit size_t
. When an addon tries to create a N-API string that contains (2^32 + 1) characters – not knowing that that doesn’t work, because, how would it tell? – N-API would currently silently truncate that size argument to 1, and happily create a one-character string. I think that qualifies as a bug.
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 think I'm still missing something here, sorry. If we change to be:
if (location_len != NAPI_AUTO_LENGTH)
is it that there is a problem later on ?
Make checks directly against NAPI_AUTO_LENGTH Add assert Remove one check in unrelated napi function that was causing a warning and is not needed
Pushed change to address comments (I think) |
New ci run to make sure assert validates we have the right behavior on all platforms: https://ci.nodejs.org/job/node-test-pull-request/10206/ |
Latest CI run was good, only failure is known pre-existing problem on centos |
auto str_maybe = v8::String::NewFromUtf8( \ | ||
(env)->isolate, (str), v8::NewStringType::kInternalized, (len)); \ | ||
(env)->isolate, (str), v8::NewStringType::kInternalized, \ | ||
static_cast<int>(len)); \ |
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.
@mhdawson The issue remains that this static_cast
is invalid if len > INT_MAX && len != NAPI_AUTO_LENGTH
, there needs to code that checks for that condition and errors out if it is met … if necessary, I guess that can be done in a subsequent PR, but it’s a bug that ought to be fixed.
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.
@addaleax thanks for the clarification, will look at that. Since we want to have the breaking changes in before 8.6 and we are almost out of time I may do it in a follow up PR, but I'll see on Monday.
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 think this check may do the trick
RETURN_STATUS_IF_FALSE((env), len <= INT_MAX, (napi_invalid_arg));
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 do think you need to check for NAPI_AUTO_LENGTH
because that value is usually larger than INT_MAX
:) But yes, something like that sounds good
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.
@addaleax agreed I also need to check for NAPI_AUTO_LENGTH. I'm going to land this PR to make sure we don't fix the Tuesday cutoff, and I'll submit a separate one to fix the bug caused by the cast on Monday.
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.
To be clear, the current check does not have well-defined behavior. Compiler A might do the right thing, compiler B might do something completely different. IOW, this is not release-ready code.
PR-URL: #15508 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Landed as a406a32 Will leave open until I open follow on PR on monday. |
PR to add check on length #15611 |
PR-URL: #15508 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs/node#15508 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#15508 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Backport-PR-URL: #19447 PR-URL: #15508 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)