[v6.x] src: some refactoring of node_url.cc by addaleax · Pull Request #18324 · nodejs/node

addaleax · 2018-01-23T19:12:05Z

Backport of #17470 with one extra commit for making this compile with clang on OS X (otherwise these are clean cherry-picks).

@MylesBorins

- 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 <timothygu99@gmail.com>

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 <timothygu99@gmail.com>

- 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 <timothygu99@gmail.com>

`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 <timothygu99@gmail.com>

This would require C++11 features that would otherwise not be available on clang + OS X on Node 6.x.

MylesBorins · 2018-01-23T19:23:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12690/

v6.x-staging CI for comparison: https://ci.nodejs.org/job/node-test-commit/15628/

MylesBorins · 2018-01-24T00:31:33Z

Looks like the Linter + AIX + linux failures are for real

linter

14:26:44 Running C++ linter...
14:26:44 src/node_url.cc:103:  Is this a non-const reference? If so, make const or use a pointer: std::string& string  [runtime/references] [2]
14:26:44 src/node_url.cc:109:  Is this a non-const reference? If so, make const or use a pointer: std::string& string  [runtime/references] [2]
14:26:44 Total errors found: 2
14:26:44 gmake: *** [Makefile:823: lint-cpp] Error 1
14:26:44 + cat test-eslint.tap
14:26:44 + sed '/^\s*$/d'
14:26:44 + grep -v '^ok\|^TAP version 13\|^1\.\.'

bsd

not ok 1179 parallel/test-tls-socket-close
  ---
  duration_ms: 0.650
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED (Signal: 11)

aix

  ...
not ok 154 parallel/test-cli-syntax
  ---
  duration_ms: 60.132
  severity: fail
  stack: |-
    timeout
  ...

These failures look unrelated though, so I'm going to dig in and see if these are perhaps known flakes...

edit: can't figure out why ci is failing on these... no prior art

addaleax · 2018-01-24T00:35:35Z

@MylesBorins Fixed the linter bits. The TLS failure in particular is a bit worrying, yes…

TimothyGu · 2018-02-01T00:54:45Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/12866/

- 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 <timothygu99@gmail.com>

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 <timothygu99@gmail.com>

- 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 <timothygu99@gmail.com>

`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 <timothygu99@gmail.com>

This would require C++11 features that would otherwise not be available on clang + OS X on Node 6.x. PR-URL: #18324 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

MylesBorins · 2018-02-05T17:03:23Z

landed in 45cb79d...6632a45

MylesBorins · 2018-02-05T17:12:01Z

btw the tls failure is unrelated and fixed in 75fbd51