Refactor pchar_parse() to use switch() #10398

ipylypiv · 2017-03-31T05:59:33Z

Comparisons c == '$' and c == '&' are present two times inside if statement.

grpc-kokoro · 2017-03-31T06:00:05Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:00:15Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:00:23Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:01:04Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:02:04Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:04:04Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:05:04Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:05:11Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:05:17Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:06:04Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:07:03Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:09:03Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:09:09Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2017-03-31T06:09:16Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

ctiller · 2017-03-31T13:13:06Z

Jenkins: this is ok to test

ctiller · 2017-03-31T13:13:35Z

src/core/ext/client_channel/uri_parser.c

-      (c == '!' || c == '$' || c == '&' || c == '\'' || c == '$' || c == '&' ||
-       c == '(' || c == ')' || c == '*' || c == '+' || c == ',' || c == ';' ||
-       c == '=') /* sub-delims */) {
+      (c == '!' || c == '$' || c == '&' || c == '\'' ||


This is begging to be converted to a switch..

or some form of LUT

Yes, agree. I will work on it. Should I create new pull request for that?

grpc-kokoro · 2017-03-31T14:58:13Z

No significant performance differences

ctiller · 2017-03-31T18:27:14Z

Could you run tools/distrib/clang-format-code.sh against this and push an update? The formatting is off compared to what our tooling expects. (you'll need docker installed)

ipylypiv · 2017-04-01T06:38:25Z

Sorry about formatting. Fixed.

grpc-kokoro · 2017-04-01T07:24:12Z

No significant performance differences

ctiller · 2017-04-01T15:09:19Z

Feel free to amend this one... No need for a new pull request

…

On Sat, Apr 1, 2017, 1:48 AM Igor Pylypiv ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/core/ext/client_channel/uri_parser.c <#10398 (comment)>: > @@ -96,9 +96,9 @@ static size_t parse_pchar(const char *uri_text, size_t i) { if (((c >= 'A') && (c <= 'Z')) || ((c >= 'a') && (c <= 'z')) || ((c >= '0') && (c <= '9')) || (c == '-' || c == '.' || c == '_' || c == '~') || /* unreserved */ - (c == '!' || c == '$' || c == '&' || c == '\'' || c == '$' || c == '&' || - c == '(' || c == ')' || c == '*' || c == '+' || c == ',' || c == ';' || - c == '=') /* sub-delims */) { + (c == '!' || c == '$' || c == '&' || c == '\'' || Yes, agree. I will work on it. Should I create new pull request for that? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#10398 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJpudfC6IpmU0CSFFSBQ2ehY_2oUHeaJks5rrg92gaJpZM4MvQWM> .

ipylypiv · 2017-04-03T07:11:04Z

During refactoring I have found few issues:

missed '@' and ':' characters. By description in comment they are also valid pchars
pchar = unreserved / pct-encoded / sub-delims / ":" / "@"
loop for (j = i + 1; j < 2; j++) executes only once and only when '%' is at position 0 (but intended to be executed twice for each '%')
check for end of string if (uri_text[i + 1] == 0 || uri_text[i + 2] == 0) is redundant because 0 ('\0') is not valid hex and this case is handled in the next if statement

Question:
Should we also reject pct-encoded 0 ('\0') byte - %00?
OWASP Null Byte Injection

Would be one additional check:
if (uri_text[i + 1] == '0' && uri_text[i + 2] == '0') return NOT_SET;

grpc-kokoro · 2017-04-03T07:44:17Z

No significant performance differences

ipylypiv · 2017-04-03T18:14:35Z

I am investigating why test failed, and will update my commit to fix that.

grpc-kokoro · 2017-04-03T20:42:35Z

No significant performance differences

grpc-kokoro · 2017-04-03T20:53:00Z

No significant performance differences

ctiller · 2017-04-04T21:45:05Z

This looks good: can you merge with head (the file moved) - and re-run clang-format?
Then I'll trigger tests again and see if we can't merge.

ipylypiv · 2017-04-04T22:03:24Z

Sure, I will merge it.

What about %00 handling?

Question:
Should we also reject pct-encoded 0 ('\0') byte - %00?
OWASP Null Byte Injection

Would be one additional check:
if (uri_text[i + 1] == '0' && uri_text[i + 2] == '0') return NOT_SET;

Add previously missed '@' and ':' characters

grpc-kokoro · 2017-04-05T04:18:49Z

Performance differences noted:
Benchmark                                                        allocs_per_iteration    atm_add_per_iteration    atm_cas_per_iteration      cpu_time  locks_per_iteration      real_time  writes_per_iteration
---------------------------------------------------------------  ----------------------  -----------------------  -----------------------  ----------  ---------------------  -----------  ----------------------
BM_HpackEncoderEncodeHeader<SingleInternedBinaryElem<1>>/0/16k                                                                                  -9.50                               -9.50
BM_HpackEncoderEncodeHeader<SingleInternedBinaryElem<31>>/0/16k                                                                                -11.50                              -11.50
BM_HpackEncoderEncodeHeader<SingleInternedBinaryElem<3>>/0/16k                                                                                  -9.50                               -9.50
BM_HpackEncoderEncodeHeader<SingleInternedElem>/0/16k                                                                                          -27.50                              -27.50

ctiller · 2017-04-05T21:50:31Z

We use slices throughout for these things, and I think we have a use case down the line for nulls (anonymous uds sockets on Linux) -- unless there's a better standard representation of these.

ctiller · 2017-04-05T21:50:39Z

Jenkins: test this please

grpc-kokoro · 2017-04-05T22:49:22Z

No significant performance differences

ipylypiv · 2017-04-10T17:00:16Z

I don't correlate latest build failures with my change. Do I need to do something else here?

googlebot added the cla: yes label Mar 31, 2017

ctiller self-requested a review March 31, 2017 13:13

ctiller self-assigned this Mar 31, 2017

ctiller reviewed Mar 31, 2017

View reviewed changes

ipylypiv changed the title ~~Remove duplicated comparisons from if statement~~ Refactor pchar_parse() to use switch() Apr 3, 2017

Refactor pchar_parse() to use switch()

282c139

Add previously missed '@' and ':' characters

ctiller approved these changes Apr 14, 2017

View reviewed changes

ctiller merged commit 88ce393 into grpc:master Apr 14, 2017

ipylypiv deleted the fix_uri_parser branch April 14, 2017 22:10

lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019

Refactor pchar_parse() to use switch() #10398

Refactor pchar_parse() to use switch() #10398

Conversation

ipylypiv commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

grpc-kokoro commented Mar 31, 2017

ctiller commented Mar 31, 2017

ctiller Mar 31, 2017

Choose a reason for hiding this comment

ctiller Mar 31, 2017

Choose a reason for hiding this comment

ipylypiv Apr 1, 2017

Choose a reason for hiding this comment

grpc-kokoro commented Mar 31, 2017

ctiller commented Mar 31, 2017

ipylypiv commented Apr 1, 2017

grpc-kokoro commented Apr 1, 2017

ctiller commented Apr 1, 2017 via email

ipylypiv commented Apr 3, 2017 • edited Loading

grpc-kokoro commented Apr 3, 2017

ipylypiv commented Apr 3, 2017

grpc-kokoro commented Apr 3, 2017

grpc-kokoro commented Apr 3, 2017

ctiller commented Apr 4, 2017

ipylypiv commented Apr 4, 2017

grpc-kokoro commented Apr 5, 2017

ctiller commented Apr 5, 2017

ctiller commented Apr 5, 2017

grpc-kokoro commented Apr 5, 2017

ipylypiv commented Apr 10, 2017

ipylypiv commented Apr 3, 2017 •

edited

Loading