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.

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

v8: add a js class for Serializer/Dserializer #13541

Closed
wants to merge 6 commits into from

Conversation

zimbabao
Copy link
Contributor

@zimbabao zimbabao commented Jun 8, 2017

Calling Serializer/Deserlizer without new crashes node.
Adding a js class which just inherits cpp bindings.

Fixes: #13326

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes (test failing on master too)
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

Calling Serializer/Deserlizer without new crashes node.
Adding a js class which just inherits cpp bindings.

Fixes: nodejs#13326
@bnoordhuis
Copy link
Member

Can you add some regression tests?

There were some gotchas around new.target and native code but I forgot the details. It would be interesting to see if class S extends v8.Serializer works and what instance.method.call({}) does.

I expect it throws an 'illegal invocation' exception but it would be good to verify that.

@addaleax
Copy link
Member

addaleax commented Jun 8, 2017

interesting to see if class S extends v8.Serializer works

It does, we already extend it :)

@bnoordhuis
Copy link
Member

I mean when you extend the class that this PR introduces.

@TimothyGu
Copy link
Member

TimothyGu commented Jun 8, 2017

@bnoordhuis, the classes this PR introduces are already extended further down the file by DefaultSerializer and DefaultDeserializer.

@bnoordhuis
Copy link
Member

Okay. Regression tests for the other issues would still be good though.

@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Jun 8, 2017
Calling Serializer/Deserlizer without new crashes node.
Adding a js class which just inherits cpp bindings.

Fixes: nodejs#13326
@zimbabao
Copy link
Contributor Author

zimbabao commented Jun 8, 2017

@bnoordhuis : Added regression tests.

@@ -131,3 +131,19 @@ const objects = [

assert.deepStrictEqual(v8.deserialize(buf), expectedResult);
}

{
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use assert.throws() for these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig : Made suggested changes.

Calling Serializer/Deserlizer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added refression tests.

Fixes: nodejs#13326
v8.Serializer();
},
Error,
"Class constructor Serializer cannot be invoked without 'new'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do something more like this:

assert.throws(
  () => { v8.Serializer(); },
  /^Error: Class constructor Serializer cannot be invoked without 'new'$/
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig : Done

@zimbabao zimbabao force-pushed the fix-serlializer-crash branch from 88817fa to b72ec9c Compare June 8, 2017 18:25
@mscdex
Copy link
Contributor

mscdex commented Jun 8, 2017

There are typos in the commit message(s): s/Dserializer/Deserializer/

@zimbabao zimbabao force-pushed the fix-serlializer-crash branch from b72ec9c to cfb934e Compare June 8, 2017 20:54
@zimbabao
Copy link
Contributor Author

zimbabao commented Jun 8, 2017

@mscdex : Thanks, fixed

@zimbabao
Copy link
Contributor Author

zimbabao commented Jun 8, 2017

Can somebody fire a CI job for this.

@mscdex
Copy link
Contributor

mscdex commented Jun 8, 2017

s/refression/regression/ in commit message(s) too.

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

Calling Serializer/Deserializer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added regression tests.

Fixes: nodejs#13326
@zimbabao zimbabao force-pushed the fix-serlializer-crash branch from cfb934e to 9643c6c Compare June 8, 2017 23:20
@zimbabao
Copy link
Contributor Author

zimbabao commented Jun 8, 2017

@mscdex : Fixed on top review.

@mscdex
Copy link
Contributor

mscdex commented Jun 9, 2017

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Still LGTM

lib/v8.js Outdated
const { copy } = process.binding('buffer');
const { objectToString } = require('internal/util');
const { FastBuffer } = require('internal/buffer');

class Serializer extends serdesBindings.Serializer {}

class Deserializer extends serdesBindings.Deserializer {}
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer something like:

const { Serializer: _Serializer, Deserializer: _Deserializer } = process.binding('serdes');
// ...

class Serializer extends _Serializer { }
class Deserializer extends _Deserializer { }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will resend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell : Made changes and added a note.

() => { v8.Deserializer(); },
/^TypeError: Class constructor Deserializer cannot be invoked without 'new'$/
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should likely also add a note to the documentation clarifying the requirement

Calling Serializer/Deserializer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added regression tests.

Fixes: nodejs#13326
@zimbabao zimbabao force-pushed the fix-serlializer-crash branch from 47cccbe to 955a1bb Compare June 9, 2017 17:57
lib/v8.js Outdated
@@ -15,11 +15,21 @@
'use strict';

const { Buffer } = require('buffer');
const { Serializer, Deserializer } = process.binding('serdes');
const {
Serializer: _Serializer,
Copy link
Member

Choose a reason for hiding this comment

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

Can you indent two spaces only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. BTW jslint didn't catch that error. May be we should add a rule?

Calling Serializer/Deserializer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added regression tests.

Fixes: nodejs#13326
@TimothyGu
Copy link
Member

@zimbabao
Copy link
Contributor Author

Tests pass on my mac osx sierra laptop. Above failures looks related to CI, can somebody start another run?.

@addaleax
Copy link
Member

Yea, CI is a bit problematic right now, I’m not sure another run would solve much. This code isn’t platform-specific, though, so I think this is okay.

@zimbabao
Copy link
Contributor Author

@addaleax : Thanks, anything else for landing this?.

@addaleax
Copy link
Member

@zimbabao Our rules say that we’ll need to wait until tomorrow to land this, but nothing from your side I think. :)

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM % docs

@refack
Copy link
Contributor

refack commented Jun 10, 2017

@addaleax
Copy link
Member

Landed in 12fd63d, thanks for the PR!

@addaleax addaleax closed this Jun 12, 2017
addaleax pushed a commit that referenced this pull request Jun 12, 2017
Calling Serializer/Deserializer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added regression tests.

Fixes: #13326
PR-URL: #13541
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 12, 2017
Calling Serializer/Deserializer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added regression tests.

Fixes: #13326
PR-URL: #13541
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax addaleax mentioned this pull request Jun 12, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
ExE-Boss added a commit to ExE-Boss/node that referenced this pull request Dec 17, 2020
addaleax pushed a commit that referenced this pull request Dec 22, 2020
Fixes: #13326
Refs: #13541
PR-URL: #36549
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
Fixes: #13326
Refs: #13541
PR-URL: #36549
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
Fixes: #13326
Refs: #13541
PR-URL: #36549
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[8.0.0] v8.Serializer crashes on bad input
8 participants