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 decoding base64 with whitespace by seishun · Pull Request #13660 · nodejs/node

Conversation

@seishun

@seishun seishun commented Jun 13, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

max_i should also include the characters that were just read by
base64_decode_group_slow.

Fixes: #13636
Fixes: #13657

Please do review the commit message as well.

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)

buffer

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 Jun 13, 2017
@seishun
seishun requested review from addaleax and aqrln June 13, 2017 15:34
@mscdex

mscdex commented Jun 13, 2017

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/8636/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Jun 13, 2017
View reviewed changes
aqrln
aqrln approved these changes Jun 13, 2017
View reviewed changes
cjihrig
cjihrig approved these changes Jun 14, 2017
View reviewed changes
@seishun

seishun commented Jun 14, 2017

Copy link
Copy Markdown
Contributor Author

I've replaced "buffer" with "src" in the commit message since this code isn't used just for the buffer module. Any objections?
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@seishun seishun changed the title buffer: fix decoding base64 with whitespace src: fix decoding base64 with whitespace Jun 14, 2017
@seishun seishun mentioned this pull request Jun 14, 2017
src: replace macro with lambda #13676
Closed
2 tasks done
@addaleax

addaleax commented Jun 16, 2017

Copy link
Copy Markdown
Member

Sorry for being so late – can you also include the patch from #13636?
All reactions

Sorry, something went wrong.

Uh oh!

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

seishun commented Jun 16, 2017

Copy link
Copy Markdown
Contributor Author

I'm having reservations about including it here, it seems barely related, and would probably confuse someone looking at the commit.

Besides, that patch isn't a proper fix for the test since null char is a valid byte. The right thing would be to compare the decoded length with the expected length.
All reactions

Sorry, something went wrong.

Uh oh!

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

addaleax commented Jun 16, 2017

Copy link
Copy Markdown
Member

I'm having reservations about including it here, it seems barely related, and would probably confuse someone looking at the commit.

You can totally include it as a separate commit, if that helps. (If you don’t want to include it, fine by me – but we should fix the test in any case.)

Besides, that patch isn't a proper fix for the test since null char is a valid byte.

Right, good point. I’d be okay with accepting that because our expected texts don’t contain null chars, but if you have reservations I can do a PR myself.
All reactions

Sorry, something went wrong.

Uh oh!

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

seishun commented Jun 16, 2017

Copy link
Copy Markdown
Contributor Author

because our expected texts don’t contain null chars

Right, they don't, but what if our base64_decode implementation starts erroneously padding its output with nulls? The test should fail then.

I can do a PR myself.

That would be great.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@seishun seishun closed this Jun 16, 2017
@seishun
seishun merged commit 64812f5 into nodejs:master Jun 16, 2017
@seishun
seishun deleted the fix-base64 branch June 16, 2017 16:11
addaleax pushed a commit that referenced this pull request Jun 17, 2017
@seishun @addaleax
src: fix decoding base64 with whitespace
07fcc14 
`max_i` should also include the characters that were just read by
`base64_decode_group_slow()`.

PR-URL: #13660
Fixes: #13636
Fixes: #13657
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax addaleax mentioned this pull request Jun 17, 2017
v8.2.0 proposal #13744
Merged
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@seishun @addaleax
src: fix decoding base64 with whitespace
a27a35b 
`max_i` should also include the characters that were just read by
`base64_decode_group_slow()`.

PR-URL: #13660
Fixes: #13636
Fixes: #13657
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax addaleax mentioned this pull request Jun 21, 2017
v8.1.3 proposal #13861
Merged
@MylesBorins MylesBorins added the lts-watch-v6.x label Jul 17, 2017
@MylesBorins

MylesBorins commented Jul 17, 2017

Copy link
Copy Markdown
Contributor

Should this be backported to v6.x?
All reactions

Sorry, something went wrong.

Uh oh!

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

seishun commented Jul 17, 2017

Copy link
Copy Markdown
Contributor Author

If it applies cleanly, then yes.
All reactions

Sorry, something went wrong.

Uh oh!

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

addaleax commented Jul 18, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

If not, we should probably still backport the test piece here. (edit: to be clear, that passes on its own on 6.11.1)
All reactions

Sorry, something went wrong.

Uh oh!

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

aqrln commented Jul 21, 2017

Copy link
Copy Markdown
Contributor

@MylesBorins this PR fixes a bug introduced in #12146, so it depends on whether that patch was backported or not.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels Aug 14, 2017
@MylesBorins

MylesBorins commented Aug 14, 2017

Copy link
Copy Markdown
Contributor

opted to not land, please feel free to manually backport the test
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@cjihrig cjihrig cjihrig approved these changes
@addaleax addaleax Awaiting requested review from addaleax
+1 more reviewer
@aqrln aqrln aqrln approved these changes
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.

base64 decoding drops characters when there is whitespace in the middle Making base64 cctest deterministic makes it fail

Uh oh!

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

8 participants

@seishun @mscdex @addaleax @MylesBorins @aqrln @jasnell @cjihrig @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.