src: remove calls to deprecated v8 functions (Uint32Value) #22143

ryzokuken · 2018-08-05T21:21:34Z

Remove all calls to deprecated v8 functions (here:
Value::Uint32Value) inside the code (src directory only).

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-08-05T21:21:35Z

@ryzokuken build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/438/pipeline

targos · 2018-08-26T19:07:21Z

src/node_buffer.cc

+  Local<Context> ctx = env->context();
+  uint32_t start, end;
+  if (!args[2]->Uint32Value(ctx).To(&start) ||
+      !args[3]->Uint32Value(ctx).To(&end))


I think you can use .As<Uint32>() for both arguments here. They are coerced in JS with value >>> 0.

targos · 2018-08-26T19:08:33Z

src/node_buffer.cc

@@ -1003,7 +1009,7 @@ void IndexOfNumber(const FunctionCallbackInfo<Value>& args) {
  THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
  SPREAD_BUFFER_ARG(args[0], ts_obj);

-  uint32_t needle = args[1]->Uint32Value();
+  uint32_t needle = args[1].As<v8::Uint32>()->Value();


nit: add using v8::Uint32; at the top of the namespace.

Also, if you are sure this is a Uint32, modify the CHECK above to IsUint32

targos · 2018-08-26T19:12:37Z

src/node_crypto.cc

-      static_cast<point_conversion_form_t>(args[0]->Uint32Value());
+  uint32_t val;
+  if (!args[0]->Uint32Value(env->context()).To(&val))
+    return env->ThrowError("Failed to parse argument");


Do we need to throw an error here?

targos · 2018-08-26T19:12:45Z

src/node_crypto.cc

-      static_cast<point_conversion_form_t>(args[2]->Uint32Value());
+  uint32_t val;
+  if (!args[2]->Uint32Value(env->context()).To(&val))
+    return env->ThrowError("Failed to parse argument");


targos · 2018-08-26T19:23:41Z

src/tcp_wrap.cc

@@ -180,7 +180,7 @@ void TCPWrap::SetKeepAlive(const FunctionCallbackInfo<Value>& args) {
                          args.Holder(),
                          args.GetReturnValue().Set(UV_EBADF));
  int enable = args[0]->Int32Value();
-  unsigned int delay = args[1]->Uint32Value();
+  unsigned int delay = args[1].As<v8::Uint32>()->Value();


This value is not verified in JS

targos · 2018-09-01T18:57:59Z

Ping. This is ready for review.

targos · 2018-09-01T18:59:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16924/

addaleax · 2018-09-01T19:40:35Z

src/node_buffer.cc

-    int value = args[1]->Uint32Value() & 255;
-    memset(ts_obj_data + start, value, fill_length);
+    uint32_t val;
+    if (args[1]->Uint32Value(ctx).To(&val)) {


Fwiw, I still find it easier to read when using the return-on-error style rather than an if (success) { … } block

Me too. Fixed.

targos · 2018-09-02T07:43:50Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/16932/

Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only).

targos · 2018-09-02T08:42:05Z

It looks like the value is not always an integer... let me check

targos · 2018-09-02T08:57:10Z

I'm lost here... It seems to me that start and end are guaranteed to be uint32.

I added this just before the binding is called:

process._rawDebug(`${start}, ${end}`);`

Last few lines of output:

16, 32
0, 10
0, 10
./node[25340]: ../src/node_buffer.cc:572:void node::Buffer::{anonymous}::Fill(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `args[2]->IsUint32()' failed.

Here is the relevant code:

node/lib/buffer.js

Lines 869 to 888 in 59e5a39

    
           if (start === undefined) { 
        
             start = 0; 
        
             end = buf.length; 
        
           } else { 
        
             // Invalid ranges are not set to a default, so can range check early. 
        
             if (end === undefined) { 
        
               if (start < 0) 
        
                 throw new ERR_INDEX_OUT_OF_RANGE(); 
        
               end = buf.length; 
        
             } else { 
        
               if (start < 0 || end > buf.length || end < 0) 
        
                 throw new ERR_INDEX_OUT_OF_RANGE(); 
        
               end = end >>> 0; 
        
             } 
        
             start = start >>> 0; 
        
             if (start >= end) 
        
               return buf; 
        
           } 
        
           const res = bindingFill(buf, val, start, end, encoding);

targos · 2018-09-02T09:00:46Z

Here is the failing block:

node/test/parallel/test-buffer-fill.js

Lines 320 to 325 in 59e5a39

    
           { 
        
             const buf = Buffer.alloc(10, 'abc'); 
        
             assert.strictEqual(buf.toString(), 'abcabcabca'); 
        
             buf.fill('է'); 
        
             assert.strictEqual(buf.toString(), 'էէէէէ'); 
        
           }

addaleax · 2018-09-02T09:03:13Z

I don’t know if it’s relevant, but there seems to be a bunch of compiler warnings from this PR:

../src/node_zlib.cc: In static member function ‘static void node::{anonymous}::ZCtx::Init(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_zlib.cc:471:119: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]
     CHECK((strategy == Z_FILTERED || strategy == Z_HUFFMAN_ONLY ||
                                                                                                                       ^               
In file included from ../src/node_zlib.cc:23:0:
../src/node_buffer.h: In static member function ‘static void node::{anonymous}::ZCtx::Write(const v8::FunctionCallbackInfo<v8::Value>&) [with bool async = true]’:
../src/node_buffer.h:75:3: warning: ‘out_off’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   if (off > max)
   ^~
../src/node_zlib.cc:174:28: note: ‘out_off’ was declared here
     size_t in_off, in_len, out_off, out_len;
                            ^~~~~~~
In file included from ../src/node_zlib.cc:23:0:
../src/node_buffer.h:79:3: warning: ‘out_len’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   if (max - off < len)
   ^~
../src/node_zlib.cc:174:37: note: ‘out_len’ was declared here
     size_t in_off, in_len, out_off, out_len;
                                     ^~~~~~~
../src/node_zlib.cc: In static member function ‘static void node::{anonymous}::ZCtx::Init(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_zlib.cc:471:85: warning: ‘strategy’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     CHECK((strategy == Z_FILTERED || strategy == Z_HUFFMAN_ONLY ||
                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                  ^               
../src/node_zlib.cc:469:14: note: ‘strategy’ was declared here
     uint32_t strategy;
              ^~~~~~~~
In file included from ../src/node_zlib.cc:23:0:
../src/node_buffer.h: In static member function ‘static void node::{anonymous}::ZCtx::Write(const v8::FunctionCallbackInfo<v8::Value>&) [with bool async = false]’:
../src/node_buffer.h:75:3: warning: ‘out_off’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   if (off > max)
   ^~
../src/node_zlib.cc:174:28: note: ‘out_off’ was declared here
     size_t in_off, in_len, out_off, out_len;
                            ^~~~~~~
In file included from ../src/node_zlib.cc:23:0:
../src/node_buffer.h:79:3: warning: ‘out_len’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   if (max - off < len)
   ^~
../src/node_zlib.cc:174:37: note: ‘out_len’ was declared here
     size_t in_off, in_len, out_off, out_len;
                                     ^~~~~~~

addaleax · 2018-09-02T09:09:48Z

@targos I think this is the failing block:

node/test/parallel/test-buffer-fill.js

Lines 327 to 330 in 59e5a39

    
           // Testing process.binding. Make sure "start" is properly checked for -1 wrap 
        
           // around. 
        
           assert.strictEqual( 
        
             process.binding('buffer').fill(Buffer.alloc(1), 1, -1, 0, 1), -2);

targos · 2018-09-02T09:54:05Z

@addaleax Haha of course, thanks!

I think I completely forgot to run the tests locally on this PR (even before making my own changes), sorry.

This should be fixed now.

CI: https://ci.nodejs.org/job/node-test-pull-request/16933/

addaleax

Still LGTM :)

addaleax · 2018-09-02T09:59:41Z

src/node_zlib.cc

+    Environment* env = ctx->env();
+    Local<Context> context = env->context();
+
+    unsigned int flush;


I guess CI is going to tell us whether it’s a real problem, but at least in theory this would be uint32_t, too

lundibundi

LGTM.
Though perhaps it's worth adding a macro for converting to uint32?
something like (not sure for the exact macro as I don't have a lot of experience with them and they are quite tricky)

#define CHECKED_TO_UINT32(ctx, from, variable)                               \
  uint32_t variable;                                                         \
  if (!(from)->Uint32Value((ctx)).To(&variable)) return;


CHECKED_TO_UINT32(ctx, args[2], start);

(Though this is not needed if we go with FromMaybe version)

lundibundi · 2018-09-02T12:51:26Z

src/node_buffer.cc

+  uint32_t start;
+  if (!args[2]->Uint32Value(ctx).To(&start)) return;
+  uint32_t end;
+  if (!args[3]->Uint32Value(ctx).To(&end)) return;


Not sure but perhaps FromMaybe version is clearer?

uint32_t start = args[2]->Uint32Value(ctx).FromMaybe(0); uint32_t end = args[3]->Uint32Value(ctx).FromMaybe(0);

Though this will result in function actually continuing execution if provided with invalid value (but wasn't this how it was before - Uint32Value() returns 0 on invalid values AFAIK? )?

Though this will result in function actually continuing execution if provided with invalid value

That is the previous behaviour, which is buggy in that it will swallow exceptions if both calls fail.

🤔 shouldn't this then be semver-major to avoid possible breakage?

Also, perhaps we can

if (!args[2]->IsUint32() || !args[3]->IsUint32()) return args.GetReturnValue().Set(-2); uint32_t start = args[2].As<Uint32>()->Value(); uint32_t end = args[3].As<Uint32>()->Value();

in this case (tests seem to pass)?

The only place where we pass in invalid arguments is from a process.binding() test. We could/should probably remove that test in a follow-up PR, and revert 05aa50f (which is pretty close to your suggestion)

Oh, I thought that this was expected somewhere else, therefore the test. The suggested commit is even better, with the above I wanted to somehow mitigate that test.

Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only). PR-URL: #22143 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>

Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only). Co-authored-by: Michaël Zasso <[email protected]> PR-URL: nodejs#22143 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>

targos · 2018-09-02T14:34:00Z

Landed in 167dd36

Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only). PR-URL: #22143 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 5, 2018

targos reviewed Aug 26, 2018

View reviewed changes

targos force-pushed the v8-deprecation-7 branch from a80ba86 to 3e400a2 Compare August 29, 2018 14:52

addaleax approved these changes Sep 1, 2018

View reviewed changes

ryzokuken and others added 5 commits September 2, 2018 10:38

src: remove calls to deprecated v8 functions (Uint32Value)

4f11f8f

Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only).

fixup! some nits

14afa7d

fixup! always uint32

ccfe17d

fixup! typo

b19df2e

fixup! return-on-error

7cdabd5

targos added 4 commits September 2, 2018 11:15

fix warnings in zlib

f8219c4

fixup! revert to casting in Buffer::Fill

05aa50f

fixup! coerce to uint32 before calling indexOf

58156f9

fixup! zlib

0bf15c5

targos force-pushed the v8-deprecation-7 branch from 0163064 to 0bf15c5 Compare September 2, 2018 09:54

addaleax approved these changes Sep 2, 2018

View reviewed changes

lundibundi approved these changes Sep 2, 2018

View reviewed changes

targos closed this Sep 2, 2018

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: remove calls to deprecated v8 functions (Uint32Value) #22143

src: remove calls to deprecated v8 functions (Uint32Value) #22143

ryzokuken commented Aug 5, 2018

nodejs-github-bot commented Aug 5, 2018

targos Aug 26, 2018

targos Aug 26, 2018

targos Aug 26, 2018

targos Aug 26, 2018

targos Aug 26, 2018

targos Aug 26, 2018

targos commented Sep 1, 2018

targos commented Sep 1, 2018

addaleax Sep 1, 2018

targos Sep 2, 2018

targos commented Sep 2, 2018

targos commented Sep 2, 2018

targos commented Sep 2, 2018

targos commented Sep 2, 2018

addaleax commented Sep 2, 2018

addaleax commented Sep 2, 2018

targos commented Sep 2, 2018 •

edited

Loading

addaleax left a comment

addaleax Sep 2, 2018

lundibundi left a comment •

edited

Loading

lundibundi Sep 2, 2018 •

edited

Loading

addaleax Sep 2, 2018

lundibundi Sep 2, 2018

addaleax Sep 2, 2018

lundibundi Sep 2, 2018

targos commented Sep 2, 2018

src: remove calls to deprecated v8 functions (Uint32Value) #22143

src: remove calls to deprecated v8 functions (Uint32Value) #22143

Conversation

ryzokuken commented Aug 5, 2018

Checklist

nodejs-github-bot commented Aug 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Sep 1, 2018

targos commented Sep 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Sep 2, 2018

targos commented Sep 2, 2018

targos commented Sep 2, 2018

targos commented Sep 2, 2018

addaleax commented Sep 2, 2018

addaleax commented Sep 2, 2018

targos commented Sep 2, 2018 • edited Loading

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi left a comment • edited Loading

Choose a reason for hiding this comment

lundibundi Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Sep 2, 2018

targos commented Sep 2, 2018 •

edited

Loading

lundibundi left a comment •

edited

Loading

lundibundi Sep 2, 2018 •

edited

Loading