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

writeVarint with a string argument can result in an invalid message #52

Closed
kjvalencik opened this issue Jun 30, 2016 · 2 comments
Closed

Comments

@kjvalencik
Copy link
Collaborator

Example:

message Envelope {
    int32 type = 1;
    string name = 2;
}
/* ... */
var Envelope = compile(proto).Envelope;
var pbf = new Pbf();

Envelope.write({
    type: 'foo',
    name: 'test'
});

Envelope.read(new Pbf(pbf.finish()))

Result: Error: Unimplemented type: 4

It seems to be due to val being being coerced into NaN. I have not tested if other number types are affected.

A simple solution would be to force it to always be a number:

// Current
val = +val;

// Replacement
val = +val || 0;

But, maybe it would be better to if (isNaN(val)) throw new Error()?

@kjvalencik
Copy link
Collaborator Author

Oddly enough, while testing adding a throw, I found that isNaN is really slow.

This reduces performance in the encode benchmark by about 25%.

if (isNaN(val)) throw new Error('Expected number');

On the other hand, this has almost no impact:

if (!(val < 0) && !(val >= 0)) throw new Error('Expected number');

@mourner
Copy link
Member

mourner commented Jun 30, 2016

I'd prefer val || 0 — we don't want to care about validating input too much, because the main priority of this library is performance and simplicity.

@mourner mourner changed the title Writing a string to a varint field can result in a message that cannot be deserialized writeVarint with a string argument can result in an invalid message Jul 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants