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.

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

address of h2o_pathconf_t should not change when more paths are added #2254

Merged
merged 4 commits into from Feb 8, 2020

Conversation

i110
Copy link
Contributor

@i110 i110 commented Feb 5, 2020

Fix #2234

hostconf->paths may be reallocated in h2o_config_register_path, but currently mruby handler has its reference. This PR fixes it by registering all paths at first, then configuring all paths

@@ -272,7 +272,7 @@ static mrb_value build_constants(mrb_state *mrb, const char *server_name, size_t
mrb_ary_set(mrb, ary, i, lit);
}
for (; i != H2O_MAX_TOKENS * 2; ++i) {
const h2o_token_t *token = h2o__tokens + i - H2O_MAX_TOKENS;
const h2o_token_t *token = h2o__tokens + (i - H2O_MAX_TOKENS);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is not related to what this PR wanted to fix, but undefined behavior sanitizer warns without this parenthesis because h2o__tokens + i overflows.

@i110 i110 mentioned this pull request Feb 5, 2020
@i110 i110 requested a review from kazuho February 5, 2020 16:56
…y already be other paths (ex: path for http2-debug-state)
@kazuho
Copy link
Member

kazuho commented Feb 6, 2020

Thank you for filing the pull request.

Am I understanding correctly that the issue is that the a handler might remember the address of h2o_pathconf_t, which might change when more paths are added?

Assuming that is the case, I think we should consider what the contract we want to have between libh2o and applications (with the standalone server being a user of libh2o in this context), rather than resolving the problem by modifying the order in which we call the functions from the configurator.

Specifically, I think the question is if applications can assume the address of h2o_pathconf_t * to be stable once it is registered.

If the answer is no, I think we should stop returning h2o_pathconf_t * from h2o_config_register_path, and rather start returning an index (or void).

If the answer is yes, we should change the way the list of paths are maintained, so that the addresses of paths do not change once each of them is registered.

@i110
Copy link
Contributor Author

i110 commented Feb 6, 2020

if applications can assume the address of h2o_pathconf_t * to be stable once it is registered.

I think the ideal answer is Yes, they should be not changed. So I commited 7172457

@i110 i110 force-pushed the i110/mruby-pathconf-dangling-pointer branch from 7172457 to 97168a5 Compare February 7, 2020 02:18
@i110 i110 merged commit 0745968 into master Feb 8, 2020
@i110 i110 deleted the i110/mruby-pathconf-dangling-pointer branch February 8, 2020 04:42
@kazuho kazuho changed the title two phase path configure address of h2o_pathconf_t should not change when more paths are added Feb 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in H2O.next.call(env)
2 participants