Skip to content

Implement "Followup improvements for ext/uri" RFC - WHATWG URL percent-encoding#22820

Open
kocsismate wants to merge 1 commit into
php:masterfrom
kocsismate:url-percent-encode
Open

Implement "Followup improvements for ext/uri" RFC - WHATWG URL percent-encoding#22820
kocsismate wants to merge 1 commit into
php:masterfrom
kocsismate:url-percent-encode

Conversation

@kocsismate

Copy link
Copy Markdown
Member

#define LXB_URL_BUFFER_NUM_SIZE 128


typedef enum {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

these changes to lexbor will be proposed upstream after the first review iteration

}

memcpy(lexbor_custom_url_map, lxb_url_map, sizeof(lxb_url_map));
lexbor_custom_url_map[37] = -1; /* % is percent-encoded */

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the custom URL map is needed in two places:

  • to percent-encode % in all modes - WHATWG doesn't try to percent-encode %
  • to percent-encode / in the PathSegment mode - this is a custom percent-encoding mode

Comment thread ext/uri/php_uri.c
default: ZEND_UNREACHABLE();
}

if (str == NULL) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As far as I saw, this is not a realistic scenario, since apparently only memory errors could lead to this code path, but memory errors bail out instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

And P.S. @ndossche Would enabling ASAN support for Lexbor be useful via the LEXBOR_HAVE_ADDRESS_SANITIZER flag, wouldn't it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Filed #22826, but I didn't have time to actually test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant