This repository has been archived by the owner. It is now read-only.
Permalink
Browse files

buffers: honor length argument in base64 decoder

Honor the length argument in `buf.write(s, 0, buf.length, 'base64')`. Before
this commit, the length argument was ignored. The decoder would keep writing
until it hit the end of the buffer. Since most buffers in Node are slices of
a parent buffer (the slab), this bug would overwrite the content of adjacent
buffers.

The bug is trivially demonstrated with the following test case:

    var assert = require('assert');
    var a = Buffer(3);
    var b = Buffer('xxx');
    a.write('aaaaaaaa', 'base64');
    assert.equal(b.toString(), 'xxx');

This commit coincidentally also fixes a bug where Buffer._charsWritten was not
updated for zero length buffers.
  • Loading branch information...
bnoordhuis committed Feb 1, 2012
1 parent 67cd054 commit f101f7c9babb31f077c78b52de7cc45ad687f57e
Showing with 40 additions and 44 deletions.
  1. +28 −43 src/node_buffer.cc
  2. +12 −1 test/simple/test-buffer.js
View
@@ -584,17 +584,6 @@ Handle<Value> Buffer::AsciiWrite(const Arguments &args) {
Handle<Value> Buffer::Base64Write(const Arguments &args) {
HandleScope scope;
- assert(unbase64('/') == 63);
- assert(unbase64('+') == 62);
- assert(unbase64('T') == 19);
- assert(unbase64('Z') == 25);
- assert(unbase64('t') == 45);
- assert(unbase64('z') == 51);
-
- assert(unbase64(' ') == -2);
- assert(unbase64('\n') == -2);
- assert(unbase64('\r') == -2);
-
Buffer *buffer = ObjectWrap::Unwrap<Buffer>(args.This());
if (!args[0]->IsString()) {
@@ -604,67 +593,52 @@ Handle<Value> Buffer::Base64Write(const Arguments &args) {
String::AsciiValue s(args[0]->ToString());
size_t offset = args[1]->Int32Value();
-
- // handle zero-length buffers graciously
- if (offset == 0 && buffer->length_ == 0) {
- return scope.Close(Integer::New(0));
- }
+ size_t max_length = args[2]->IsUndefined() ? buffer->length_ - offset
+ : args[2]->Uint32Value();
+ max_length = MIN(s.length(), MIN(buffer->length_ - offset, max_length));
if (offset >= buffer->length_) {
return ThrowException(Exception::TypeError(String::New(
"Offset is out of bounds")));
}
- const size_t size = base64_decoded_size(*s, s.length());
- if (size > buffer->length_ - offset) {
- // throw exception, don't silently truncate
- return ThrowException(Exception::TypeError(String::New(
- "Buffer too small")));
- }
-
char a, b, c, d;
char* start = buffer->data_ + offset;
char* dst = start;
- const char *src = *s;
- const char *const srcEnd = src + s.length();
+ char* const dstEnd = dst + max_length;
+ const char* src = *s;
+ const char* const srcEnd = src + s.length();
- while (src < srcEnd) {
+ while (src < srcEnd && dst < dstEnd) {
int remaining = srcEnd - src;
- while (unbase64(*src) < 0 && src < srcEnd) {
- src++;
- remaining--;
- }
+ while (unbase64(*src) < 0 && src < srcEnd) src++, remaining--;
if (remaining == 0 || *src == '=') break;
a = unbase64(*src++);
- while (unbase64(*src) < 0 && src < srcEnd) {
- src++;
- remaining--;
- }
+ while (unbase64(*src) < 0 && src < srcEnd) src++, remaining--;
if (remaining <= 1 || *src == '=') break;
b = unbase64(*src++);
+
*dst++ = (a << 2) | ((b & 0x30) >> 4);
+ if (dst == dstEnd) break;
- while (unbase64(*src) < 0 && src < srcEnd) {
- src++;
- remaining--;
- }
+ while (unbase64(*src) < 0 && src < srcEnd) src++, remaining--;
if (remaining <= 2 || *src == '=') break;
c = unbase64(*src++);
+
*dst++ = ((b & 0x0F) << 4) | ((c & 0x3C) >> 2);
+ if (dst == dstEnd) break;
- while (unbase64(*src) < 0 && src < srcEnd) {
- src++;
- remaining--;
- }
+ while (unbase64(*src) < 0 && src < srcEnd) src++, remaining--;
if (remaining <= 3 || *src == '=') break;
d = unbase64(*src++);
+
*dst++ = ((c & 0x03) << 6) | (d & 0x3F);
}
constructor_template->GetFunction()->Set(chars_written_sym,
- Integer::New(s.length()));
+ Integer::New(dst - start));
return scope.Close(Integer::New(dst - start));
}
@@ -759,6 +733,17 @@ bool Buffer::HasInstance(v8::Handle<v8::Value> val) {
void Buffer::Initialize(Handle<Object> target) {
HandleScope scope;
+ // sanity checks
+ assert(unbase64('/') == 63);
+ assert(unbase64('+') == 62);
+ assert(unbase64('T') == 19);
+ assert(unbase64('Z') == 25);
+ assert(unbase64('t') == 45);
+ assert(unbase64('z') == 51);
+ assert(unbase64(' ') == -2);
+ assert(unbase64('\n') == -2);
+ assert(unbase64('\r') == -2);
+
length_symbol = Persistent<String>::New(String::NewSymbol("length"));
chars_written_sym = Persistent<String>::New(String::NewSymbol("_charsWritten"));
View
@@ -687,7 +687,7 @@ assert.equal(Buffer._charsWritten, 9);
buf.write('0123456789', 'binary');
assert.equal(Buffer._charsWritten, 9);
buf.write('123456', 'base64');
-assert.equal(Buffer._charsWritten, 6);
+assert.equal(Buffer._charsWritten, 4);
buf.write('00010203040506070809', 'hex');
assert.equal(Buffer._charsWritten, 18);
@@ -703,3 +703,14 @@ assert.equal(Buffer({length: 'BAM'}).length, 0);
// Make sure that strings are not coerced to numbers.
assert.equal(Buffer('99').length, 2);
assert.equal(Buffer('13.37').length, 5);
+
+// Ensure that the length argument is respected.
+'ascii utf8 hex base64 binary'.split(' ').forEach(function(enc) {
+ assert.equal(Buffer(1).write('aaaaaa', 0, 1, enc), 1);
+});
+
+// Regression test, guard against buffer overrun in the base64 decoder.
+var a = Buffer(3);
+var b = Buffer('xxx');
+a.write('aaaaaaaa', 'base64');
+assert.equal(b.toString(), 'xxx');

0 comments on commit f101f7c

Please sign in to comment.