Skip to content

Do not copy the normalized URI when cloning RFC 3986 URIs - alternative approach #19744

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

Closed
wants to merge 1 commit into from
+15 −8
Closed

Do not copy the normalized URI when cloning RFC 3986 URIs - alternative approach #19744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion ext/uri/php_uri.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -990,7 +990,13 @@ zend_object *uri_clone_obj_handler(zend_object *object)

new_uri_object->internal.parser = internal_uri->parser;

void *uri = internal_uri->parser->clone_uri(internal_uri->uri);
zend_execute_data *execute_data = EG(current_execute_data);
bool is_clone_op = execute_data != NULL &&
execute_data->func &&
ZEND_USER_CODE(execute_data->func->type) &&
execute_data->opline != NULL &&
execute_data->opline->opcode == ZEND_CLONE;
Copy link
Member

Choose a reason for hiding this comment

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

Please no, I object against these kinds of hacks. This might also not work well with the clone-as-function feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

:( alright

Copy link
Member Author

Choose a reason for hiding this comment

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

is there any other solution you can see that achieves similar result? The only thing I can is to set a global variable before cloning that the RFC 3986 implementation could check. But I guess this is also going to be considered hacky :)

Copy link
Member

Choose a reason for hiding this comment

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

I would assume that uri_clone_obj_handler() is always called by clone, and introduce a separate function with extra arguments for other use-cases. uri_clone_obj_handler() can call the new function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is a reasonable assumption, however Tim has just changed the code of the withers to call the clone obj handler instead of directly calling uri_clone_obj_handler() (https://github.com/php/php-src/pull/19649/files#diff-5b8085788a05a46216af5a9b0c9db7e47351d41f3550952d497ba3d54922be15L92). He did this so that the chance of calling the improper clone obj handler is eliminated, and this also makes sense at the first sight, but now that I think about it again, uri_write_component_ex is not public code, and it's only used by PHP's own implementations... So I guess we could assume that uri_clone_obj_handler() can be called directly.

Copy link
Member

Choose a reason for hiding this comment

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

It is still unclear to me, why we try to add this amount of complexity to the C code, when my previous suggestion of “just don't clone the normalized URI, because it's very likely to be invalidated” would be obviously correct. As part of my cleanup and review of ext/uri, I've fixed more one one case of memory unsafety.

Copy link
Member

Choose a reason for hiding this comment

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

You're doing too much premature optimization here. Go for the simple solution of Tim where we just don't copy the normalized URI. It's going to be much easier and less error-prone.
If you find in a (realistic) benchmark that it does matter, you can come up with a solution. But keep it simple at first and only when numbers show it you should optimize.

Copy link
Member Author

@kocsismate kocsismate Sep 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The change to properly copy normalized URIs when needed didn't seem complex to me: we could go back using uri_clone_obj_handler() inside uri_write_component_ex() and then we could pass false/true depending on the use-case (by using Arnaud's suggestion).

Yes, this improves the situation for only a small minority of use-cases, but for them, it's a huge improvement not to have to reparse the whole URI. Of course, real world benchmarks won't reveal this, unless I deliberately start cloning URIs like crazy for whatever reason.

In my opinion, this warrants the extra care. Not missing this opportunity seems more vital for me than optimizing other smaller details like converting zend_string_release() calls to zend_string_release_ex().

With all that said, if you both agree that we shouldn't take care of this usecase, then I'll accept and respect your decision, and close this PR.

Copy link
Member

@TimWolla TimWolla Sep 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

than optimizing other smaller details like converting zend_string_release() calls to zend_string_release_ex().

FWIW: I also agree with that. For me it's needless mental load that all these variants exist and just always use zend_string_release(). Doing C correctly itself is already complicated enough.

Copy link
Member

Choose a reason for hiding this comment

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

Ah and what I also wanted to add: One issue I'm having here is that this would be adding another parameter to the API that only matters for one of the implementations, similarly to the errors parameter existing only for WhatWg. The current implementation tries to use generic helpers for something that differs in detail, leading to combinatorial explosion of (boolean) parameters in those helpers.

void *uri = internal_uri->parser->clone_uri(internal_uri->uri, is_clone_op == false);
Copy link
Member Author

Choose a reason for hiding this comment

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

This way the extra check affects all URI parsers - so the above check could be directly added to the clone_uri handler of RFC 3986... 🤔

ZEND_ASSERT(uri != NULL);

new_uri_object->internal.uri = uri;
Expand Down
3 changes: 2 additions & 1 deletion ext/uri/php_uri_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,10 @@ typedef struct uri_parser_t {
*
* A deep-clone must be performed that copies all pointer members to a new memory address.
* @param uri The input URI
* @param is_modified_after_cloning If the handler is called by a wither method that modifies the URI
* @return The cloned URI
*/
void *(*clone_uri)(void *uri);
void *(*clone_uri)(void *uri, bool is_modified_after_cloning);

/**
* Recomposes a URI as a string according to the recomposition_mode and exclude_fragment parameters.
Expand Down
10 changes: 5 additions & 5 deletions ext/uri/uri_parser_rfc3986.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,16 +353,16 @@ void *php_uri_parser_rfc3986_parse(const char *uri_str, size_t uri_str_len, cons
return php_uri_parser_rfc3986_parse_ex(uri_str, uri_str_len, base_url, silent);
}

/* When calling a wither successfully, the normalized URI is surely invalidated, therefore
* it doesn't make sense to copy it. In case of failure, an exception is thrown, and the URI object
* is discarded altogether. */
ZEND_ATTRIBUTE_NONNULL static void *php_uri_parser_rfc3986_clone(void *uri)
ZEND_ATTRIBUTE_NONNULL static void *php_uri_parser_rfc3986_clone(void *uri, bool is_modified_after_cloning)
{
const php_uri_parser_rfc3986_uris *uriparser_uris = uri;

php_uri_parser_rfc3986_uris *new_uriparser_uris = uriparser_create_uris();
copy_uri(&new_uriparser_uris->uri, &uriparser_uris->uri);
if (uriparser_uris->normalized_uri_initialized) {
/* Only copy the normalized URI if cloning won't immediately modify the URI,
* thus invalidating the normalized URI. This is the case when the call comes from
* a wither method. */
if (is_modified_after_cloning == false && uriparser_uris->normalized_uri_initialized) {
copy_uri(&new_uriparser_uris->normalized_uri, &uriparser_uris->normalized_uri);
new_uriparser_uris->normalized_uri_initialized = true;
}
Expand Down
2 changes: 1 addition & 1 deletion ext/uri/uri_parser_whatwg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ static void *php_uri_parser_whatwg_parse(const char *uri_str, size_t uri_str_len
return php_uri_parser_whatwg_parse_ex(uri_str, uri_str_len, base_url, errors, silent);
}

static void *php_uri_parser_whatwg_clone(void *uri)
static void *php_uri_parser_whatwg_clone(void *uri, bool is_modified_after_cloning)
{
const lxb_url_t *lexbor_uri = uri;

Expand Down