GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: fix base64 decoding by seishun · Pull Request #11995 · nodejs/node

Conversation

@seishun

@seishun seishun commented Mar 22, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Make sure trailing garbage is not treated as a valid Base64 character.

Fixes: #11987

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

src

cc @bnoordhuis @jorangreef

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Mar 22, 2017
@seishun

seishun commented Mar 22, 2017

Copy link
Copy Markdown
Contributor Author

Do let me know if someone has a better idea for the test.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Mar 22, 2017
View reviewed changes
Comment thread test/parallel/test-buffer-alloc.js Outdated

@addaleax addaleax Mar 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think a good test would be

assert.deepStrictEqual(Buffer.from('w0  ', 'base64'), Buffer.from('w0', 'base64'));

and/or checking the actual bytes?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jorangreef jorangreef Mar 24, 2017

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks @seishun and @addaleax.

The test is good. Agree on checking the actual bytes:

assert.deepStrictEqual(Buffer.from('w0  ', 'base64'), Buffer.from([195]));

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Mar 24, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Both of the suggested tests are worthwhile...

assert.deepStrictEqual(Buffer.from('w0  ', 'base64'), Buffer.from('w0', 'base64'));
assert.deepStrictEqual(Buffer.from('w0  ', 'base64'), Buffer.from([195]));

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jorangreef reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@targos targos Mar 24, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

And also from the issue:

assert.strictEqual(
  Buffer.from('3v6YpUFyK0/hitA2tCDIfdYKw0 g ', 'base64').toString('base64'),
  '3v6YpUFyK0/hitA2tCDIfdYKw0g='
);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jorangreef reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@addaleax addaleax added lts-watch-v4.x labels Mar 23, 2017
@jasnell jasnell added this to the 8.0.0 milestone Mar 27, 2017
@addaleax

addaleax commented Mar 31, 2017

Copy link
Copy Markdown
Member

@seishun Took the liberty of pushing to your branch, hope that’s okay. :) Rebased & tests added from comments.

CI: https://ci.nodejs.org/job/node-test-commit/8812/

I’m going to land this if the CI comes back green. @aqrln Heads up, this is going to conflict with your PRs. ;)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
seishun
seishun commented Mar 31, 2017
View reviewed changes
Comment thread test/parallel/test-buffer-alloc.js Outdated

@seishun seishun Mar 31, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@addaleax I think adding all these tests is redundant. This one conveys intent the best:

assert.deepStrictEqual(Buffer.from('w0  ', 'base64'),
                       Buffer.from('w0', 'base64'));

If this test passes but one of the other proposed tests fails, that means base64 encoding is fundamentally broken, which would cause one of the many existing tests to fail.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Mar 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@seishun It’s your PR and I’m not going to tell you what to do. 😄 But really, having slightly more tests than necessary won’t hurt anyone…

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@addaleax

addaleax commented Mar 31, 2017

Copy link
Copy Markdown
Member

@seishun CI is green … I’d say you can merge this with any non-empty subset of the tests here added. I’d prefer all but every assertion here should be sufficient as a regression test for the particular issue this fixes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aqrln

aqrln commented Mar 31, 2017

Copy link
Copy Markdown
Contributor

@addaleax thanks for the heads up! It'll conflict with only one of the PRs, and the conflict will be easy to fix, so I'll rebase it as soon as this one lands :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@seishun
src: fix base64 decoding
7e0c3ab 
Make sure trailing garbage is not treated as a valid base64 character.

Fixes: #11987
PR-URL: #11995
Reviewed-By: Anna Henningsen <[email protected]>
@seishun

seishun commented Mar 31, 2017

Copy link
Copy Markdown
Contributor Author

Landed in 7e0c3ab.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@seishun seishun closed this Mar 31, 2017
@seishun
seishun merged commit 7e0c3ab into nodejs:master Mar 31, 2017
@seishun
seishun deleted the base64-fix branch March 31, 2017 20:02
@aqrln aqrln mentioned this pull request Mar 31, 2017
buffer: optimize decoding wrapped base64 data #12146
Closed
3 tasks done
@jasnell jasnell mentioned this pull request Apr 4, 2017
8.0.0 Release Proposal #12220
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
@seishun Italo A. Casas
src: fix base64 decoding
ffbcfdf 
Make sure trailing garbage is not treated as a valid base64 character.

Fixes: nodejs#11987
PR-URL: nodejs#11995
Reviewed-By: Anna Henningsen <[email protected]>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
v7.9.0 Release Proposal #12319
Merged
2 tasks done
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
@seishun @MylesBorins
src: fix base64 decoding
6aeaf21 
Make sure trailing garbage is not treated as a valid base64 character.

Fixes: #11987
PR-URL: #11995
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins

MylesBorins commented Apr 18, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Cherry-picked to v6. Needed manual backport for v4: 83a856af6d

please lmk if you see any issues
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Apr 18, 2017
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@seishun @MylesBorins
src: fix base64 decoding
eb393f9 
Make sure trailing garbage is not treated as a valid base64 character.

Fixes: #11987
PR-URL: #11995
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@seishun @MylesBorins
src: fix base64 decoding
1a63321 
Make sure trailing garbage is not treated as a valid base64 character.

Fixes: #11987
PR-URL: #11995
Reviewed-By: Anna Henningsen <[email protected]>
This was referenced Apr 19, 2017
v6.10.3 proposal #12498
Merged
v4.8.3 proposal #12499
Merged
MylesBorins added a commit that referenced this pull request May 2, 2017
@MylesBorins
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
788f280 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12499
MylesBorins added a commit that referenced this pull request May 2, 2017
@MylesBorins
2017-05-02, Version 6.10.3 'Boron' (LTS)
a0bfd4f 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12497
MylesBorins added a commit that referenced this pull request May 2, 2017
@MylesBorins
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
9802d46 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12499
MylesBorins added a commit that referenced this pull request May 2, 2017
@MylesBorins
2017-05-02, Version 6.10.3 'Boron' (LTS)
20e27dd 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12497
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
@imyller
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
9c2a002 
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12499

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
@imyller
2017-05-02, Version 6.10.3 'Boron' (LTS)
6c7c224 
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12497

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
@imyller
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
27a8c43 
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12499

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
@imyller
2017-05-02, Version 6.10.3 'Boron' (LTS)
3d61075 
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12497

Signed-off-by: Ilkka Myller <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
@MylesBorins Olivier Martin
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
38a7454 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) nodejs#9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) nodejs#11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) nodejs#11947
   * (Ben Noordhuis) nodejs#11898
   * (jBarz) nodejs#11776

PR-URL: nodejs#12499
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
@MylesBorins Olivier Martin
2017-05-02, Version 6.10.3 'Boron' (LTS)
f25ca7a 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) nodejs#9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) nodejs#11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) nodejs#11947
   * (Ben Noordhuis) nodejs#11898
   * (jBarz) nodejs#11776

PR-URL: nodejs#12497
@addaleax addaleax mentioned this pull request Jun 1, 2017
result of Buffer.from with base64 is different between versions nodejs/help#651
Closed
@seishun seishun mentioned this pull request Jun 14, 2017
src: replace macro with lambda #13676
Closed
2 tasks done
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@seishun @andrew749
src: fix base64 decoding
ad9318b 
Make sure trailing garbage is not treated as a valid base64 character.

Fixes: nodejs/node#11987
PR-URL: nodejs/node#11995
Reviewed-By: Anna Henningsen <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@MylesBorins @andrew749
2017-05-02, Version 6.10.3 'Boron' (LTS)
94b893c 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) nodejs/node#9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) nodejs/node#11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) nodejs/node#11947
   * (Ben Noordhuis) nodejs/node#11898
   * (jBarz) nodejs/node#11776

PR-URL: nodejs/node#12497
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments
@addaleax addaleax addaleax approved these changes
+2 more reviewers
@jorangreef jorangreef jorangreef left review comments
@targos targos targos left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Buffer.from('base64') decodes some white space as data

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@seishun @addaleax @aqrln @MylesBorins @jorangreef @jasnell @targos @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.