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

Buffer.alloc v4: Incorrectly returns zero-filled buffer when encoding is passed #9226

Closed
CrabDude opened this issue Oct 21, 2016 · 7 comments
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@CrabDude
Copy link

CrabDude commented Oct 21, 2016

  • v4.6.1, 4.5.0, 4.6.0:
  • Darwin Kernel Version 16.0.0; root:xnu-3789.1.32~3/RELEASE_X86_64 x86_64:
  • Buffer.alloc(size, data, enc):
$ node
> let s = 'YQ=='
undefined
> Buffer.alloc(s.length, s, 'base64')
<Buffer 00 00 00 00>
> Buffer(s, 'base64')
<Buffer 61>
> Buffer.alloc(1, s, 'base64')
<Buffer 00>
> Buffer(s, 'base64').toString('base64')
'YQ=='
> let b = Buffer(s.length); b.write(s, 'base64'); b
<Buffer 61 00 00 00>
> b.toString('base64')
'YQAAAA=='

Also, the Buffer.alloc(size, data, enc) docs example fails:

> Buffer.alloc(11, 'aGVsbG8gd29ybGQ=', 'base64');
<Buffer 00 00 00 00 00 00 00 00 00 00 00>
@mscdex mscdex added buffer Issues and PRs related to the buffer subsystem. v4.x labels Oct 21, 2016
@mscdex
Copy link
Contributor

mscdex commented Oct 21, 2016

/cc @nodejs/buffer

@jasnell jasnell added the confirmed-bug Issues with confirmed bugs. label Oct 21, 2016
@not-an-aardvark
Copy link
Contributor

Working on this. The issue is that Buffer.alloc calls Buffer.fill with another Buffer as an argument. However, passing another Buffer as an argument is not supported in Node 4.

not-an-aardvark added a commit to not-an-aardvark/node that referenced this issue Oct 22, 2016
Previously, the implementation of Buffer.alloc() called Buffer#fill()
with another Buffer as an argument. However, in v4.x, Buffer#fill does
not support a Buffer as a parameter. As a workaround, call
binding.fill() directly in the Buffer.alloc() implementation.

Fixes: nodejs#9226
@ChALkeR
Copy link
Member

ChALkeR commented Oct 23, 2016

Ow, that was my backport. Btw, it probably means that there is no testcase for that in master, if it wasn't added recently, as I backported with testcases. Will take a look at that.

@ChALkeR
Copy link
Member

ChALkeR commented Oct 23, 2016

Update: the coverage was recently (2016-09-19) fixed in master in 8699ecd (#8552). That probably needs to be backported to v4.x.

@MylesBorins
Copy link
Contributor

@ChALkeR would you be able to do a Pr?

On Sun, Oct 23, 2016, 10:45 AM Сковорода Никита Андреевич <
[email protected]> wrote:

Update: the coverage was recently (2016-09-19) fixed in master in 8699ecd
8699ecd
(#8552 #8552). That probably needs
to be backported to v4.x.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#9226 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecV-1819JoJtStID1Du2txIiEqX0dQks5q2yymgaJpZM4Kdfcn
.

@ChALkeR
Copy link
Member

ChALkeR commented Oct 25, 2016

@thealphanerd You mean the testcases update? I will take a look at that.

MylesBorins pushed a commit that referenced this issue Oct 26, 2016
Previously, the implementation of Buffer.alloc() called Buffer#fill()
with another Buffer as an argument. However, in v4.x, Buffer#fill does
not support a Buffer as a parameter. As a workaround, call
binding.fill() directly in the Buffer.alloc() implementation.

Fixes: #9226
PR-URL: #9238
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

fixed in dc3e45f and will land in v4.6.2

MylesBorins pushed a commit that referenced this issue Oct 26, 2016
Previously, the implementation of Buffer.alloc() called Buffer#fill()
with another Buffer as an argument. However, in v4.x, Buffer#fill does
not support a Buffer as a parameter. As a workaround, call
binding.fill() directly in the Buffer.alloc() implementation.

Fixes: #9226
PR-URL: #9238
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

No branches or pull requests

6 participants