Skip to content

src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid #46227

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.

Sign up for GitHub

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

Merged
nodejs-github-bot merged 14 commits into from
Jan 23, 2023
+54 −14
Merged

src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid #46227

Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ff9c478
src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid
miguelteixeiraa Jan 16, 2023
f076935
src: fix node_url.cc code formatting
miguelteixeiraa Jan 16, 2023
b1f4c83
src: fix node_url.cc code formatting (back to what it was)
miguelteixeiraa Jan 16, 2023
95d3081
src: refactor EndsInANumber in node_url.cc to use string_view::rfind
miguelteixeiraa Jan 17, 2023
e9282ad
src: fix imports typo in node_url.cc
miguelteixeiraa Jan 17, 2023
a683d34
src: removes unnecessary if statement from EndsInANumber in node_url.cc
miguelteixeiraa Jan 17, 2023
d165e39
src: use string_view instead of string in EndsInANumber in node_url.cc
miguelteixeiraa Jan 17, 2023
573dd4a
src: fix string_view constructor call for IsIPv4NumberValid in node_u…
miguelteixeiraa Jan 17, 2023
b52bbd4
src: adds minor tweaks to EndsInANumber at node_url.cc
miguelteixeiraa Jan 18, 2023
3074383
src: adds comment to IsIPv4NumberValid conditions in node_url.cc
miguelteixeiraa Jan 18, 2023
de4f065
src: cast interators to size_t in IsIPv4NumberValid call in node_url.cc
miguelteixeiraa Jan 18, 2023
9058eb5
src: cast interator to const char* in IsIPv4NumberValid call in node_…
miguelteixeiraa Jan 18, 2023
b65769f
src: use &(*it) to make windows build pass + ::all_of instead of ::fi…
miguelteixeiraa Jan 19, 2023
9534455
src: refactor EndsInANumber to be more readable in node_url.cc
miguelteixeiraa Jan 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 47 additions & 14 deletions src/node_url.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,29 +407,62 @@ int64_t ParseIPv4Number(const char* start, const char* end) {
return strtoll(start, nullptr, R);
}

// https://url.spec.whatwg.org/#ipv4-number-parser
bool IsIPv4NumberValid(std::string_view input) {
Comment thread
anonrig marked this conversation as resolved.
Outdated
if (input.empty()) {
return false;
}

if (input.size() >= 2 && input[0] == '0') {
Comment thread
anonrig marked this conversation as resolved.
if (input[1] == 'X' || input[1] == 'x') {
if (input.size() == 2) {
return true;
}

return input.find_first_not_of("0123456789abcdefABCDEF", 2) ==
Comment thread
anonrig marked this conversation as resolved.
Outdated
std::string_view::npos;
} else {
Comment thread
anonrig marked this conversation as resolved.
Outdated
return input.find_first_not_of("01234567", 1) == std::string_view::npos;
}
}

return std::all_of(input.begin(), input.end(), ::isdigit);
Comment thread
anonrig marked this conversation as resolved.
Outdated
}

// https://url.spec.whatwg.org/#ends-in-a-number-checker
bool EndsInANumber(const std::string& input) {
std::vector<std::string> parts = SplitString(input, '.', false);
bool EndsInANumber(const std::string_view input) {
Comment thread
anonrig marked this conversation as resolved.
Outdated
if (input.empty()) {
return false;
}

char delimiter = '.';
auto pointer_start = input.begin();
Copy link
Copy Markdown
Member

@anonrig anonrig Jan 17, 2023

Choose a reason for hiding this comment

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

If you use std::string_view::iterator it might solve the issues related to std::string_view construct on windows.

Copy link
Copy Markdown
Contributor Author

@miguelteixeiraa miguelteixeiraa Jan 18, 2023

Choose a reason for hiding this comment

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

done!

Copy link
Copy Markdown
Contributor Author

@miguelteixeiraa miguelteixeiraa Jan 18, 2023

Choose a reason for hiding this comment

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

Windows build is still failing : /

D:\a\node\node\src\node_url.cc(465,66): error C2440: '<function-style-cast>': cannot convert from 'initializer list' to 'std::string_view' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_url.cc(465,66): message : No constructor could take the source type, or constructor overload resolution was ambiguous [D:\a\node\node\libnode.vcxproj]
  node_util.cc

Copy link
Copy Markdown
Contributor Author

@miguelteixeiraa miguelteixeiraa Jan 18, 2023

Choose a reason for hiding this comment

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

I would like to know if I can go with &(*it); again

Copy link
Copy Markdown
Member

@anonrig anonrig Jan 18, 2023

Choose a reason for hiding this comment

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

@addaleax any recommendations?

Copy link
Copy Markdown
Contributor Author

@miguelteixeiraa miguelteixeiraa Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it'd look like this:
std::string_view(&(*pointer_start), pointer_end - pointer_start));
just to make the build pass on windows

Copy link
Copy Markdown
Contributor Author

@miguelteixeiraa miguelteixeiraa Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What i understand is that iterators don't need to be pointers although they could be implemented as one. And some implementations of the STL (MSVC++, Mingw) implement interators with special classes (with no pre-difined casts to pointer), so that we cannot use them directly in the string_view constructor that we are using, which needs a charT*.

then we could use * which is overloaded to do what logically mean: convert the iterator type to its pointed-to object, then pass the address of this &(*it).

https://en.cppreference.com/w/cpp/named_req/RandomAccessIterator
https://stackoverflow.com/questions/32654108/c-stdvectoriterator-is-not-a-pointer-why/64910815#64910815

auto pointer_end = input.end();

if (parts.empty()) return false;
uint8_t parts_size = std::count(pointer_start, pointer_end, delimiter);
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jan 18, 2023

Choose a reason for hiding this comment

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

Why uint8_t?

Copy link
Copy Markdown
Contributor Author

@miguelteixeiraa miguelteixeiraa Jan 18, 2023

Choose a reason for hiding this comment

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

Because it's an IPv4, so there shouldn't be more parts than, lets say, 4. In short.. it would always be a small number (I think uint8_t still quite big for this)
but yeah.. maybe I should change to what std::count returns so it don't mess up the number..
Do you think it should be changed?

Copy link
Copy Markdown
Member

@anonrig anonrig Jan 18, 2023

Choose a reason for hiding this comment

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

uint8_t is correct. referencing spec:

A valid IPv4-address string must be four shortest possible strings of [ASCII digits]
(https://infra.spec.whatwg.org/#ascii-digit), representing a decimal number in the 
range 0 to 255, inclusive, separated from each other by U+002E (.).

Copy link
Copy Markdown
Member

@addaleax addaleax Jan 18, 2023

Choose a reason for hiding this comment

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

maybe I should change to what std::count returns so it don't mess up the number..

I think that’s a good idea, yeah. Reading this code it’s really not obvious that anything in the code guarantees that the return value fits into an uint8_t, even if that’s a type that can hold the all valid results. As an extreme case, if input contains 256 dots, parts_size would be 0 here, and I assume we don’t want to treat that the same as a string that doesn’t contain any dots.

++parts_size;

if (parts.back().empty()) {
if (parts.size() == 1) return false;
parts.pop_back();
if (input.back() == delimiter) {
--pointer_end;
--parts_size;
}

const std::string& last = parts.back();
if (parts_size > 1) {
pointer_start +=
input.rfind(delimiter, std::distance(pointer_start, pointer_end) - 1) +
1;
}

// If last is non-empty and contains only ASCII digits, then return true
if (!last.empty() && std::all_of(last.begin(), last.end(), ::isdigit)) {
return true;
if (std::distance(pointer_start, pointer_end) == 0) {
return false;
}

const char* last_str = last.c_str();
int64_t num = ParseIPv4Number(last_str, last_str + last.size());
if (num >= 0) return true;
if (std::all_of(pointer_start, pointer_end, ::isdigit)) {
return true;
}

return false;
return IsIPv4NumberValid(
std::string_view(&*pointer_start, pointer_end - pointer_start));
Copy link
Copy Markdown
Member

@anonrig anonrig Jan 17, 2023

Choose a reason for hiding this comment

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

This is too hacky (even if it's guaranteed safe).

Copy link
Copy Markdown
Contributor Author

@miguelteixeiraa miguelteixeiraa Jan 18, 2023

Choose a reason for hiding this comment

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

removed!

Copy link
Copy Markdown
Contributor Author

@miguelteixeiraa miguelteixeiraa Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll try a static_cast<const char*>

Copy link
Copy Markdown
Member

@anonrig anonrig Jan 18, 2023

Choose a reason for hiding this comment

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

@nodejs/cpp-reviewers any recommendations?

Copy link
Copy Markdown
Member

@addaleax addaleax Jan 18, 2023

Choose a reason for hiding this comment

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

Yeah: I would definitely recommeng to go back to the original "hacky" implementation. It works and looks correct, even if it is not visually pretty. (I'd also not call this pointer if it's an iterator.) const_cast<char*>(pointer_start) doesn't even really make sense since the string_view constructor takes a const char* argument.

Copy link
Copy Markdown
Contributor Author

@miguelteixeiraa miguelteixeiraa Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I think it only works on the other compilers because their implementations for the iterator are:
std::string_view::iterator' (aka 'const char *')
(I purposely caused an error to catch this 'aka' haha)

}

void URLHost::ParseIPv4Host(const char* input, size_t length) {
Expand Down