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

URLHost not robust when re-setting its value #18302

Closed
TimothyGu opened this issue Jan 22, 2018 · 3 comments
Closed

URLHost not robust when re-setting its value #18302

TimothyGu opened this issue Jan 22, 2018 · 3 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@TimothyGu
Copy link
Member

  • Version: master
  • Platform: all
  • Subsystem: url, src

Currently, the SetOpaque() and SetDomain() methods of URLHost class in node_url.cc always overwrite the existing string in value_ without disposing of the original value in that union.

node/src/node_url.cc

Lines 95 to 112 in a3555d0

// Setting the string members of the union with = is brittle because
// it relies on them being initialized to a state that requires no
// destruction of old data.
// For a long time, that worked well enough because ParseIPv6Host() happens
// to zero-fill `value_`, but that really is relying on standard library
// internals too much.
// These helpers are the easiest solution but we might want to consider
// just not forcing strings into an union.
inline void SetOpaque(std::string&& string) {
type_ = HostType::H_OPAQUE;
new(&value_.opaque) std::string(std::move(string));
}
inline void SetDomain(std::string&& string) {
type_ = HostType::H_DOMAIN;
new(&value_.domain) std::string(std::move(string));
}
};

This could cause a memory leak when these two methods are used on an instance of the class on which one of these two methods has already been called.

Right now that never happens because of the way the URL parsing state machine is designed, but ideally these two methods should first call this->~URLHost() to free any memory already allocated before reinitializing the value through the new placement.

@TimothyGu TimothyGu added whatwg-url Issues and PRs related to the WHATWG URL implementation. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jan 22, 2018
@addaleax
Copy link
Member

this->~URLHost()

I’m just going to mention again that I’d prefer something like a Reset() method instead that we could call in these setters and in the destructor – calling the destructor of on object explicitly seems pretty icky, especially when we can avoid it. ;)

(That being said, I’d consider this worthy of being a good first issue if somebody wants to do some C++ stuff?)

@apapirovski apapirovski added good first issue Issues that are suitable for first-time contributors. mentor-available labels Jan 23, 2018
@apapirovski
Copy link
Member

I've marked this as a good first issue and mentor available. I'm happy to assist anyone that wants to take this on, it should be a very simple change.

If anyone's considering this: all that's required is to make a private Reset method on URLHost that contains the current contents of the destructor, then call it from the destructor, SetDomain & SetOpaque.

@prog1dev
Copy link
Contributor

@apapirovski Alright, I`ll do this one then :)

prog1dev added a commit to prog1dev/node that referenced this issue Jan 23, 2018
MylesBorins pushed a commit that referenced this issue Feb 20, 2018
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Mar 28, 2018
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Mar 30, 2018
Fixes: #18302

PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
prog1dev added a commit to prog1dev/node that referenced this issue Apr 1, 2018
Fixes: nodejs#18302

PR-URL: nodejs#18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Apr 13, 2018
Fixes: #18302

Backport-PR-URL: #19639
PR-URL: #18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Fixes: nodejs#18302

PR-URL: nodejs#18357
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants