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.

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

Jump to bottom

Enable decoding URIs with ipv6 address in host. #6900

Merged
merged 2 commits into from Apr 3, 2023
Merged

Enable decoding URIs with ipv6 address in host. #6900

merged 2 commits into from Apr 3, 2023

Conversation

derekkraan
Copy link
Contributor

Fixes #6898

@CLAassistant
Copy link

CLAassistant commented Feb 20, 2023 •
edited

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2023 •
edited

CT Test Results

       3 files     133 suites   47m 10s ⏱️
1 548 tests 1 497 ✔️ 51 💤 0 ❌
1 965 runs  1 895 ✔️ 70 💤 0 ❌

Results for commit eb4ef9d.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@derekkraan derekkraan mentioned this pull request Feb 20, 2023
Merged
@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Feb 20, 2023
sverker
sverker reviewed Feb 21, 2023
View reviewed changes
erts/emulator/beam/packet_parser.c Outdated Show resolved Hide resolved
erts/emulator/beam/packet_parser.c Outdated Show resolved Hide resolved
@derekkraan
Copy link
Contributor Author

Thanks for the feedback, I'll take a look this week hopefully and see if I can address your comments.

@derekkraan
Copy link
Contributor Author

@sverker I pushed another commit, this should cover the cases. Now it checks for both [ and ] in a single if statement and only parses the port out of integers coming after the ]: (now also checking for the presence of the :).

@sverker
Copy link
Contributor

sverker commented Mar 2, 2023

Sorry, a missing ] still fails hard:

3> erlang:decode_packet(http_bin, <<"GET http://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210:4000/echo_components HTTP/1.1\r\nhost: orange\r\n\r\n">>, []).
Segmentation fault (core dumped)

You will get here with p as NULL:

/* host[:port]  */
    else {
        int n = (p - uri_ptr);

Add the "missing ]" into the test case.

@derekkraan
Copy link
Contributor Author

derekkraan commented Mar 3, 2023 •
edited

You're absolutely correct. I have forgotten the intricacies of C after so many years of not using it.

We can choose between running memchr a second time (because we are overwriting p with the location of ], and we need it to represent the location of :), or adding a new variable. I chose to add a new variable. Let me know what you think.

sverker
sverker reviewed Mar 7, 2023
View reviewed changes
Copy link
Contributor

@sverker sverker left a comment

Choose a reason for hiding this comment

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

Could you write something in the documentation of erlang:decode_packet about how the Host string can be an ipv6 address within [] from OTP 26.0.

@derekkraan
Copy link
Contributor Author

Yes, I can do that.

@derekkraan
Copy link
Contributor Author

I added that requested documentation @sverker, hope it's sufficient. I also added a link to the RFC there.

@sverker sverker added feature testing currently being tested, tag is used by OTP internal CI labels Mar 24, 2023
sverker
sverker reviewed Mar 25, 2023
View reviewed changes
erts/emulator/beam/packet_parser.c Outdated
uri->s1_len = n;
n = uri_len - (n+1);
// parse port if the next char is `:`
if (memrchr(uri_ptr, ':', uri_len) == v + 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

memrchr is a GNU extension not available on Windows:
packet_parser.o : error LNK2019: unresolved external symbol memrchr referenced in function http_parse_absoluteURI

Copy link
Contributor

Choose a reason for hiding this comment

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

I will fix this

derekkraan and others added 2 commits April 3, 2023 14:19
@derekkraan @sverker
erts: Enable decoding URIs with ipv6 address in host.
9ccd064 
Example:

Packet = <<"GET http://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:4000/dir ...>>
erlang:decode_packet(http_bin, Packet, [])
@sverker
erts: Add sys_memchr()
ef1ded8 
for completeness/symmetry with sys_memrchr().
@sverker sverker force-pushed the decode_packet_ipv6_address branch from eb4ef9d to ef1ded8 Compare April 3, 2023 12:24
@sverker sverker merged commit 83a979e into erlang:master Apr 3, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sverker sverker sverker left review comments

Assignees

@sverker sverker

Labels
feature team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

erlang:decode_packet/3 doesn't support IPv6 hosts
4 participants
@derekkraan @CLAassistant @sverker @rickard-green