Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Buffer#copy() is incompatible with SlowBuffers #4633

Closed
TooTallNate opened this Issue Jan 21, 2013 · 5 comments

Comments

Projects
None yet
3 participants

Repro:

var buffer = require('buffer');
var a = new Buffer(1);
var b = new buffer.SlowBuffer(1);
a.copy(b);
buffer.js:576
  return this.parent.copy(target.parent,
                     ^
TypeError: First arg should be a Buffer
    at Buffer.copy (buffer.js:576:22)
    at Object.<anonymous> (/Users/nrajlich/buf.js:4:3)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.runMain (module.js:499:10)
    at process._tickCallback (node.js:386:13)

/cc @trevnorris

Owner

bnoordhuis commented Jan 21, 2013

Presumably trivial to fix (see below) but when exactly is that an issue?

diff --git a/lib/buffer.js b/lib/buffer.js
index d605829..d333243 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -573,7 +573,7 @@ Buffer.prototype.copy = function(target, target_start, start, end) {
     end = target.length - target_start + start;
   }

-  return this.parent.copy(target.parent,
+  return this.parent.copy(target.parent || target,
                           target_start + target.offset,
                           start + this.offset,
                           end + this.offset);

I'm dealing with a native addon that returns SlowBuffer instances directly
(fuse4js specifically).

On Monday, January 21, 2013, Ben Noordhuis wrote:

Presumably trivial to fix (see below) but when exactly is that an issue?

diff --git a/lib/buffer.js b/lib/buffer.jsindex d605829..d333243 100644--- a/lib/buffer.js+++ b/lib/buffer.js@@ -573,7 +573,7 @@ Buffer.prototype.copy = function(target, target_start, start, end) {
end = target.length - target_start + start;
}

  • return this.parent.copy(target.parent,+ return this.parent.copy(target.parent || target,
    target_start + target.offset,
    start + this.offset,
    end + this.offset);


Reply to this email directly or view it on GitHubhttps://github.com/joyent/node/issues/4633#issuecomment-12489221.

Slow buffers don't have an offset, right? So the target.offset argument will evaluate to NaN.

Slow buffers don't have an offset, right? So the target.offset argument will evaluate to NaN.

That is correct.

@TooTallNate TooTallNate added a commit that referenced this issue Jan 25, 2013

@trevnorris @TooTallNate trevnorris + TooTallNate buffer: slow buffer copy compatibility fix
Fix issue where SlowBuffers couldn't be passed as target to Buffer
copy().

Also included checks to see if Argument parameters are defined before
assigning their values. This offered ~3x's performance gain.

Backport of 16bbecc from master branch. Closes #4633.
65249cc

Thanks @trevnorris. Closed by 65249cc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment