GeezerVison

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

GeezerVison is showing github.com — src: some refactoring of node_url.cc by addaleax · Pull Request #17470 · nodejs/node

Conversation

addaleax commented Dec 5, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

src: minor cleanups to node_url.cc
- Remove pointless pointers
- Make WriteHost take a const argument so that it’s functionality
  is clear from the signature
- Make FindLongestZeroSequence templated to accommodate the constness
  in WriteHost and because using uint16_t is an articifial,
  unnecessary restriction
- Remove string copying when no copies are needed
- Make PercentDecode just return its return value
- Make ParseHost (string-only version) take its constant argument
  as a constant reference
src: move url internals into anonymous namespace

This helps because static doesn’t work for C++ classes,
but refactoring url_host into a proper C++ class seems the
most reasonable soluation for the memory leak fixed by the next commit.
src: make url host a proper C++ class
- Gives URLHost a proper destructor that clears memory
  depending on the type of the host (This fixes a memory leak)
- Hide the host type enums and class layout as implementation details
- Make the Parse methods members of URLHost
- Turn WriteHost into a ToString() method on the URLHost class
- Verify that at the beginning of a parse attempt, the type is set
  to “failed”
- Remove a lot of gotos from the source code 🐢🚀
Fixes: Why use new URL() may lead to memory leak ? #17448
src: use correct OOB check for IPv6 parsing

last_piece pointed to the end of the 8×16 bit array,
so piece_pointer == last_piece already means that the pointer
is not writable any longer.

Previously, this still worked most of the time but could
result in an out-of-bounds-write.

Also, rename last_piece to buffer_end to avoid this pitfall.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src/node_url.cc

Not sure how to write tests but it makes valgrind happy about @TimothyGu’s example from #17448

Sorry, something went wrong.

Uh oh!

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

Trott and TimothyGu reacted with hooray emoji

Trott and TimothyGu reacted with heart emoji

All reactions

🎉 2 reactions
❤️ 2 reactions

addaleax added c++

Issues and PRs that require attention from people who are familiar with C++.

whatwg-url Issues and PRs related to the WHATWG URL implementation. labels

Dec 5, 2017

addaleax requested a review from TimothyGu

December 5, 2017 13:56

nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels

Dec 5, 2017

addaleax added 2 commits

December 5, 2017 14:57


          src: minor cleanups to node_url.cc

e86f577

- Remove pointless pointers
- Make `WriteHost` take a const argument so that it’s functionality
  is clear from the signature
- Make `FindLongestZeroSequence` templated to accommodate the constness
  in `WriteHost` and because using `uint16_t` is an articifial,
  unnecessary restriction
- Remove string copying when no copies are needed
- Make `PercentDecode` just return its return value
- Make `ParseHost` (string-only version) take its constant argument
  as a constant reference


          src: move url internals into anonymous namespace

c28a3a4

This helps because `static` doesn’t work for C++ classes,
but refactoring `url_host` into a proper C++ class seems the
most reasonable soluation for the memory leak fixed by the next commit.

addaleax force-pushed the url-refactor branch from 1aa20c6 to 9fef727 Compare

December 5, 2017 13:57

addaleax commented Dec 5, 2017

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

addaleax commented Dec 5, 2017

Copy link

Copy Markdown

Member Author

Some of the test failures are definitely related & a bit curious … going to look into those later

All reactions

Sorry, something went wrong.

Uh oh!

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


          src: make url host a proper C++ class

363ad6a

- Gives `URLHost` a proper destructor that clears memory
  depending on the type of the host (This fixes a memory leak)
- Hide the host type enums and class layout as implementation details
- Make the `Parse` methods members of `URLHost`
- Turn `WriteHost` into a `ToString()` method on the `URLHost` class
- Verify that at the beginning of a parse attempt, the type is set
  to “failed”
- Remove a lot of `goto`s from the source code 🐢🚀

Fixes: nodejs#17448

addaleax force-pushed the url-refactor branch from 9fef727 to 363ad6a Compare

December 5, 2017 18:30


          src: use correct OOB check for IPv6 parsing

c8bef2d

`last_piece` pointed to the end of the 8×16 bit array,
so `piece_pointer == last_piece` already means that the pointer
is not writable any longer.

Previously, this still worked most of the time but could
result in an out-of-bounds-write.

Also, rename `last_piece` to `buffer_end` to avoid this pitfall.

addaleax commented Dec 5, 2017

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

TimothyGu approved these changes

Dec 5, 2017

View reviewed changes

TimothyGu left a comment

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.

Splendid work! Good to see this whole thing properly C++-ized, and the memory leak fixed. LGTM.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/node_url.cc

		while (ch != kEOL) {
		if (piece_pointer > last_piece)
		goto end;
		if (piece_pointer >= buffer_end)

TimothyGu Dec 5, 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.

Wow, nice catch!

Sorry, something went wrong.

Uh oh!

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

All reactions

addaleax Dec 5, 2017

Copy link

Copy Markdown

Member 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.

@TimothyGu well, it was breaking CI for me, soooo... ;)

Wouldn't have seen it without the tests breaking.

Sorry, something went wrong.

Uh oh!

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

TimothyGu reacted with laugh emoji

All reactions

😄 1 reaction

addaleax added the lts-watch-v8.x label

Dec 5, 2017

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare

December 8, 2017 16:37

addaleax added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Dec 8, 2017

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Dec 12, 2017


          src: minor cleanups to node_url.cc

2050bab

- Remove pointless pointers
- Make `WriteHost` take a const argument so that it’s functionality
  is clear from the signature
- Make `FindLongestZeroSequence` templated to accommodate the constness
  in `WriteHost` and because using `uint16_t` is an articifial,
  unnecessary restriction
- Remove string copying when no copies are needed
- Make `PercentDecode` just return its return value
- Make `ParseHost` (string-only version) take its constant argument
  as a constant reference

PR-URL: nodejs#17470
Reviewed-By: Timothy Gu <[email protected]>

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Dec 12, 2017


          src: move url internals into anonymous namespace

89b3746

This helps because `static` doesn’t work for C++ classes,
but refactoring `url_host` into a proper C++ class seems the
most reasonable soluation for the memory leak fixed by the next commit.

PR-URL: nodejs#17470
Reviewed-By: Timothy Gu <[email protected]>

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Dec 12, 2017


          src: make url host a proper C++ class

9236dfe

- Gives `URLHost` a proper destructor that clears memory
  depending on the type of the host (This fixes a memory leak)
- Hide the host type enums and class layout as implementation details
- Make the `Parse` methods members of `URLHost`
- Turn `WriteHost` into a `ToString()` method on the `URLHost` class
- Verify that at the beginning of a parse attempt, the type is set
  to “failed”
- Remove a lot of `goto`s from the source code 🐢🚀

PR-URL: nodejs#17470
Fixes: nodejs#17448
Reviewed-By: Timothy Gu <[email protected]>

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Dec 12, 2017


          src: use correct OOB check for IPv6 parsing

e8a5f7b

`last_piece` pointed to the end of the 8×16 bit array,
so `piece_pointer == last_piece` already means that the pointer
is not writable any longer.

Previously, this still worked most of the time but could
result in an out-of-bounds-write.

Also, rename `last_piece` to `buffer_end` to avoid this pitfall.

PR-URL: nodejs#17470
Reviewed-By: Timothy Gu <[email protected]>

BridgeAR commented Dec 12, 2017

Copy link

Copy Markdown

Member

Landed in e8a5f7b, 9236dfe, 89b3746, 2050bab

All reactions

Sorry, something went wrong.

Uh oh!

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

BridgeAR closed this

Dec 12, 2017

addaleax removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Dec 13, 2017

addaleax deleted the url-refactor branch

December 13, 2017 05:20

MylesBorins commented Dec 20, 2017

Copy link

Copy Markdown

Contributor

@TimothyGu does this need to be included in the backport?

All reactions

Sorry, something went wrong.

Uh oh!

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

gibfahn commented Dec 20, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

@trygve-lie , in reply to #17774 (comment)

Normally we don't backport to LTS until things have been in a current release (9.x) for at least two weeks. This hasn't gone into a 9.x release yet, so we'd be talking 6 weeks before this could land in v8.x.

If this fixes a serious bug we should consider including it in the v8.9.4 release candidate build going out today, so that it goes out with v8.9.4 in two weeks.

What would be helpful is an idea of:

How much pain this bug causes
How many people are going to hit this
How minor the fix is (how much risk is there in rushing it)

Thoughts @nodejs/lts ?

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.

addaleax added a commit to addaleax/node that referenced this pull request

Jan 23, 2018


          src: make url host a proper C++ class

2de0731

- Gives `URLHost` a proper destructor that clears memory
  depending on the type of the host (This fixes a memory leak)
- Hide the host type enums and class layout as implementation details
- Make the `Parse` methods members of `URLHost`
- Turn `WriteHost` into a `ToString()` method on the `URLHost` class
- Verify that at the beginning of a parse attempt, the type is set
  to “failed”
- Remove a lot of `goto`s from the source code 🐢🚀

PR-URL: nodejs#17470
Fixes: nodejs#17448
Reviewed-By: Timothy Gu <[email protected]>

addaleax added a commit to addaleax/node that referenced this pull request

Jan 23, 2018


          src: use correct OOB check for IPv6 parsing

b70a4b4

`last_piece` pointed to the end of the 8×16 bit array,
so `piece_pointer == last_piece` already means that the pointer
is not writable any longer.

Previously, this still worked most of the time but could
result in an out-of-bounds-write.

Also, rename `last_piece` to `buffer_end` to avoid this pitfall.

PR-URL: nodejs#17470
Reviewed-By: Timothy Gu <[email protected]>

addaleax mentioned this pull request

Jan 23, 2018

[v6.x] src: some refactoring of node_url.cc #18324

Closed

MylesBorins pushed a commit that referenced this pull request

Feb 5, 2018


          src: minor cleanups to node_url.cc

ad1a6b5

- Remove pointless pointers
- Make `WriteHost` take a const argument so that it’s functionality
  is clear from the signature
- Make `FindLongestZeroSequence` templated to accommodate the constness
  in `WriteHost` and because using `uint16_t` is an articifial,
  unnecessary restriction
- Remove string copying when no copies are needed
- Make `PercentDecode` just return its return value
- Make `ParseHost` (string-only version) take its constant argument
  as a constant reference

Backport-PR-URL: #18324
PR-URL: #17470
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Feb 5, 2018


          src: move url internals into anonymous namespace

b77162f

This helps because `static` doesn’t work for C++ classes,
but refactoring `url_host` into a proper C++ class seems the
most reasonable soluation for the memory leak fixed by the next commit.

Backport-PR-URL: #18324
PR-URL: #17470
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Feb 5, 2018


          src: make url host a proper C++ class

2a7c835

- Gives `URLHost` a proper destructor that clears memory
  depending on the type of the host (This fixes a memory leak)
- Hide the host type enums and class layout as implementation details
- Make the `Parse` methods members of `URLHost`
- Turn `WriteHost` into a `ToString()` method on the `URLHost` class
- Verify that at the beginning of a parse attempt, the type is set
  to “failed”
- Remove a lot of `goto`s from the source code 🐢🚀

Backport-PR-URL: #18324
PR-URL: #17470
Fixes: #17448
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Feb 5, 2018


          src: use correct OOB check for IPv6 parsing

8f6f09b

`last_piece` pointed to the end of the 8×16 bit array,
so `piece_pointer == last_piece` already means that the pointer
is not writable any longer.

Previously, this still worked most of the time but could
result in an out-of-bounds-write.

Also, rename `last_piece` to `buffer_end` to avoid this pitfall.

Backport-PR-URL: #18324
PR-URL: #17470
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins mentioned this pull request

Feb 7, 2018

v6.13.0 proposal #18342

Merged

MylesBorins pushed a commit that referenced this pull request

Feb 11, 2018


          src: minor cleanups to node_url.cc

c2c441e

- Remove pointless pointers
- Make `WriteHost` take a const argument so that it’s functionality
  is clear from the signature
- Make `FindLongestZeroSequence` templated to accommodate the constness
  in `WriteHost` and because using `uint16_t` is an articifial,
  unnecessary restriction
- Remove string copying when no copies are needed
- Make `PercentDecode` just return its return value
- Make `ParseHost` (string-only version) take its constant argument
  as a constant reference

Backport-PR-URL: #18324
PR-URL: #17470
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Feb 11, 2018


          src: move url internals into anonymous namespace

3dbcc8e

This helps because `static` doesn’t work for C++ classes,
but refactoring `url_host` into a proper C++ class seems the
most reasonable soluation for the memory leak fixed by the next commit.

Backport-PR-URL: #18324
PR-URL: #17470
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Feb 11, 2018


          src: make url host a proper C++ class

997045f

- Gives `URLHost` a proper destructor that clears memory
  depending on the type of the host (This fixes a memory leak)
- Hide the host type enums and class layout as implementation details
- Make the `Parse` methods members of `URLHost`
- Turn `WriteHost` into a `ToString()` method on the `URLHost` class
- Verify that at the beginning of a parse attempt, the type is set
  to “failed”
- Remove a lot of `goto`s from the source code 🐢🚀

Backport-PR-URL: #18324
PR-URL: #17470
Fixes: #17448
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Feb 11, 2018


          src: use correct OOB check for IPv6 parsing

384a029

`last_piece` pointed to the end of the 8×16 bit array,
so `piece_pointer == last_piece` already means that the pointer
is not writable any longer.

Previously, this still worked most of the time but could
result in an out-of-bounds-write.

Also, rename `last_piece` to `buffer_end` to avoid this pitfall.

Backport-PR-URL: #18324
PR-URL: #17470
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Feb 12, 2018


          src: minor cleanups to node_url.cc

5453c22

- Remove pointless pointers
- Make `WriteHost` take a const argument so that it’s functionality
  is clear from the signature
- Make `FindLongestZeroSequence` templated to accommodate the constness
  in `WriteHost` and because using `uint16_t` is an articifial,
  unnecessary restriction
- Remove string copying when no copies are needed
- Make `PercentDecode` just return its return value
- Make `ParseHost` (string-only version) take its constant argument
  as a constant reference

Backport-PR-URL: #18324
PR-URL: #17470
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Feb 12, 2018


          src: move url internals into anonymous namespace

23e04fc

This helps because `static` doesn’t work for C++ classes,
but refactoring `url_host` into a proper C++ class seems the
most reasonable soluation for the memory leak fixed by the next commit.

Backport-PR-URL: #18324
PR-URL: #17470
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Feb 12, 2018


          src: make url host a proper C++ class

8034d8f

- Gives `URLHost` a proper destructor that clears memory
  depending on the type of the host (This fixes a memory leak)
- Hide the host type enums and class layout as implementation details
- Make the `Parse` methods members of `URLHost`
- Turn `WriteHost` into a `ToString()` method on the `URLHost` class
- Verify that at the beginning of a parse attempt, the type is set
  to “failed”
- Remove a lot of `goto`s from the source code 🐢🚀

Backport-PR-URL: #18324
PR-URL: #17470
Fixes: #17448
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Feb 12, 2018


          src: use correct OOB check for IPv6 parsing

c78112d

`last_piece` pointed to the end of the 8×16 bit array,
so `piece_pointer == last_piece` already means that the pointer
is not writable any longer.

Previously, this still worked most of the time but could
result in an out-of-bounds-write.

Also, rename `last_piece` to `buffer_end` to avoid this pitfall.

Backport-PR-URL: #18324
PR-URL: #17470
Reviewed-By: Timothy Gu <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Feb 13, 2018


          src: minor cleanups to node_url.cc

d66f469

- Remove pointless pointers
- Make `WriteHost` take a const argument so that it’s functionality
  is clear from the signature
- Make `FindLongestZeroSequence` templated to accommodate the constness
  in `WriteHost` and because using `uint16_t` is an articifial,
  unnecessary restriction
- Remove string copying when no copies are needed
- Make `PercentDecode` just return its return value
- Make `ParseHost` (string-only version) take its constant argument
  as a constant reference

Backport-PR-URL: #18324
PR-URL: #17470
Reviewed-By: Timothy Gu <[email protected]>

TimothyGu mentioned this pull request

Feb 27, 2018

WHATWG URL Memory Leak #19019

Closed

Reviewers

1 more reviewer

TimothyGu

TimothyGu 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

c++

Issues and PRs that require attention from people who are familiar with C++.

whatwg-url

Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Why use new URL() may lead to memory leak ?

Uh oh!

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

8 participants

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.

GeezerVison

Conversation

addaleax commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Dec 5, 2017

Uh oh!

addaleax commented Dec 5, 2017

Uh oh!

addaleax commented Dec 5, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 12, 2017

Uh oh!

MylesBorins commented Dec 20, 2017

Uh oh!

gibfahn commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

addaleax commented Dec 5, 2017 •

edited

Loading

Uh oh!

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

gibfahn commented Dec 20, 2017 •

edited

Loading

Uh oh!

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