Skip to content

Commit

Permalink
buffer: let WriteFloatGeneric silently drop values
Browse files Browse the repository at this point in the history
Documentation currently states that setting noAssert and passing a value
larger than can fit in the Buffer will cause data to be silently
dropped. Change implementation to match documented behavior.

Fixes: #3766
Reviewed-By: Trevor Norris <[email protected]>
  • Loading branch information
pmq20 authored and trevnorris committed Nov 17, 2015
1 parent df268f9 commit 0ed3a7c
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 3 deletions.
8 changes: 5 additions & 3 deletions src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -735,7 +735,9 @@ uint32_t WriteFloatGeneric(const FunctionCallbackInfo<Value>& args) {

T val = args[1]->NumberValue();
uint32_t offset = args[2]->Uint32Value();
CHECK_LE(offset + sizeof(T), ts_obj_length);
size_t memcpy_num = sizeof(T);
if (offset + sizeof(T) > ts_obj_length)
memcpy_num = ts_obj_length - offset;

union NoAlias {
T val;
Expand All @@ -746,8 +748,8 @@ uint32_t WriteFloatGeneric(const FunctionCallbackInfo<Value>& args) {
char* ptr = static_cast<char*>(ts_obj_data) + offset;
if (endianness != GetEndianness())
Swizzle(na.bytes, sizeof(na.bytes));
memcpy(ptr, na.bytes, sizeof(na.bytes));
return offset + sizeof(na.bytes);
memcpy(ptr, na.bytes, memcpy_num);
return offset + memcpy_num;
}


Expand Down
7 changes: 7 additions & 0 deletions test/parallel/test-buffer-arraybuffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,10 @@ assert.throws(function() {
AB.prototype.__proto__ = ArrayBuffer.prototype;
new Buffer(new AB());
}, TypeError);

// write{Double,Float}{LE,BE} with noAssert should not crash, cf. #3766
var b = new Buffer(1);
b.writeFloatLE(11.11, 0, true);
b.writeFloatBE(11.11, 0, true);
b.writeDoubleLE(11.11, 0, true);
b.writeDoubleBE(11.11, 0, true);

6 comments on commit 0ed3a7c

@mscdex
Copy link
Contributor

@mscdex mscdex commented on 0ed3a7c Nov 17, 2015

Choose a reason for hiding this comment

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

This is missing the PR-URL which should be: #3767

@trevnorris
Copy link
Contributor

Choose a reason for hiding this comment

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

FFFFUUUU!!!!

Thanks @mscdex for placing the reference here.

@jasnell
Copy link
Member

Choose a reason for hiding this comment

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

I'm just happy I'm not the only one ;-)

@evanlucas
Copy link
Contributor

Choose a reason for hiding this comment

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

/cc @nodejs/punished :]

@trevnorris
Copy link
Contributor

Choose a reason for hiding this comment

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

@evanlucas yeah, i'm the only one on it. :-(

@jasnell
Copy link
Member

Choose a reason for hiding this comment

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

(heh.. they just haven't noticed any of mine yet ... ;-) ... oh, wait, did I just say that out loud)

Please sign in to comment.