src: remove calls to deprecated v8 functions (Uint32Value) by ryzokuken · Pull Request #22143 · nodejs/node · GitHub

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

@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

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

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

Do we need to throw an error here?

ditto

This value is not verified in JS

Ping. This is ready for review.

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

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

Me too. Fixed.

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).

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

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);

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(), 'էէէէէ'); 
        
           }

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;
                                     ^~~~~~~

@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);

@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/

Still LGTM :)

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

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)

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 <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

Remove all calls to deprecated v8 functions (here: Value::Uint32Value) inside the code (src directory only). Co-authored-by: Michaël Zasso <targos@protonmail.com> PR-URL: nodejs#22143 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

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

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

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

src: remove calls to deprecated v8 functions (Uint32Value) by ryzokuken · Pull Request #22143 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

src: remove calls to deprecated v8 functions (Uint32Value)#22143ryzokuken wants to merge 9 commits intonodejs:masternodejs/node:masterfrom ryzokuken:v8-deprecation-7ryzokuken/node:v8-deprecation-7Copy head branch name to clipboard

Conversation

ryzokuken commented Aug 5, 2018

Checklist

Uh oh!

nodejs-github-bot commented Aug 5, 2018

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!

targos commented Sep 1, 2018

Uh oh!

targos commented Sep 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

targos commented Sep 2, 2018

Uh oh!

targos commented Sep 2, 2018

Uh oh!

targos commented Sep 2, 2018

Uh oh!

targos commented Sep 2, 2018

Uh oh!

addaleax commented Sep 2, 2018

Uh oh!

addaleax commented Sep 2, 2018

Uh oh!

targos commented Sep 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lundibundi left a comment • 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!

lundibundi Sep 2, 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!

targos commented Sep 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

src: remove calls to deprecated v8 functions (Uint32Value)#22143
ryzokuken wants to merge 9 commits into
nodejs:masterfrom
ryzokuken:v8-deprecation-7

targos commented Sep 2, 2018 •
edited
Loading

lundibundi left a comment •
edited
Loading

lundibundi Sep 2, 2018 •
edited
Loading