Skip to content

Don't use zend_string for registerExtension- It may be freed #254

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

Closed
wants to merge 2 commits into from
Closed

Don't use zend_string for registerExtension- It may be freed #254

wants to merge 2 commits into from

Conversation

TysonAndre
Copy link
Contributor

... before module shutdown
(In NTS builds of PHP 7 (e.g. 7.0.9) only)

Fixes #247

This no longer returns getExtensions() in the order in which they were
registered. vector<pair<char*, v8js_extension*>>> could be added to reproduce the old behavior if this was desirable.

This is slightly wasteful of memory - A custom hash function and
equality function for const char* is possible to add to this for an unordered_map

Tyson Andre and others added 2 commits August 11, 2016 21:25
... before module shutdown
(In NTS builds of PHP 7 (e.g. 7.0.9) only)

Fixes #247

This no longer returns getExtensions() in the order in which they were
registered.
This is slightly wasteful of memory - A custom hash function and
equality for `const char*` might be possible.
@stesie
Copy link
Member

stesie commented Aug 12, 2016

Hi,

sorry for not answering on your issue earlier, and thanks for now taking care of it. Actually I started investigating yesterday night and noticed that it was related to interned strings - which aren't really duplicated by:

    jsext->name = zend_string_dup(name, 1);
    jsext->source = zend_string_dup(source, 1);

... hence zend_string_release in v8js_jsext_free_storage frees them (and hence the "use after free" error). So a different solution would have been to just test jsext->name and jsext->source to be interned and not free them then.

From a first glance at your patch it looks good to me - so I haven't tried it out yet. There are two "TODO" marks left in there, do you intend to continue on them?

Generally I'm fine with the change to an unordered map of extensions, and also to now use null-byte-terminated strings (I wouldn't expect anyone to have null-bytes in extension names) ...

@TysonAndre
Copy link
Contributor Author

oh, you seem to be right about interned strings being freed earlier.

Misread what you said the first time, see #255 (Converts interned strings to non-interned strings)

@TysonAndre TysonAndre closed this Aug 12, 2016
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.

2 participants