Skip to content

fix: used allocation without checking#9015

Open
hwde wants to merge 5 commits intophp:PHP-8.0from
hwde:fix_for_some_untested_allocations
Open

fix: used allocation without checking#9015
hwde wants to merge 5 commits intophp:PHP-8.0from
hwde:fix_for_some_untested_allocations

Conversation

@hwde
Copy link
Contributor

@hwde hwde commented Jul 14, 2022

No description provided.

@hwde hwde mentioned this pull request Jul 15, 2022
@hwde hwde force-pushed the fix_for_some_untested_allocations branch 3 times, most recently from 18a4241 to 3d23209 Compare July 15, 2022 11:33
@cmb69
Copy link
Member

cmb69 commented Jul 19, 2022

Thanks for the PR! This is okay, but why not use pemalloc() and friends? These are infallible, and can be used to allocate persistent memory. Also, any particular reason to target PHP-8.1? If it's a bug, the fix should target PHP-8.0; if it is an improvement, it should target "master" only.

@hwde hwde force-pushed the fix_for_some_untested_allocations branch from 3d23209 to e9a604f Compare July 20, 2022 16:32
@hwde
Copy link
Contributor Author

hwde commented Jul 20, 2022

Thanks for the PR! This is okay, but why not use pemalloc() and friends? These are infallible, and can be used to allocate persistent memory. Also, any particular reason to target PHP-8.1? If it's a bug, the fix should target PHP-8.0; if it is an improvement, it should target "master" only.

I changed to pemalloc, I would be happy if you change it to PHP-8.0 ... who knows what will happen if I try ...

@hwde
Copy link
Contributor Author

hwde commented Jul 20, 2022

btw. if I look at the define of pestrdup() :

#define pestrdup(s, persistent) ((persistent)?strdup(s):estrdup(s))

the persistent half doesn't look safe to me.

@cmb69
Copy link
Member

cmb69 commented Jul 20, 2022

I would be happy if you change it to PHP-8.0 ... who knows what will happen if I try ...

I can do, but usually its easy to do:

  • rebase locally onto the new target branch
  • force-push (with-lease)
  • change the target branch in the GH UI

the persistent half doesn't look safe to me.

Ugh!

@hwde hwde force-pushed the fix_for_some_untested_allocations branch from e9a604f to d3f332a Compare July 21, 2022 17:39
@hwde hwde changed the base branch from PHP-8.1 to PHP-8.0 July 21, 2022 17:40

if (!scope) {
scope = malloc(sizeof(zend_ffi_scope));
scope = pemalloc(sizeof(zend_ffi_scope), 1);
Copy link
Member

Choose a reason for hiding this comment

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

you might need to mirror it in their freeing counterparts.


if (!FFI_G(scopes)) {
FFI_G(scopes) = malloc(sizeof(HashTable));
FFI_G(scopes) = pemalloc(sizeof(HashTable), 1);
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

@cmb69
Copy link
Member

cmb69 commented Jul 25, 2022

the persistent half doesn't look safe to me.

I've filed #9128.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

IMO this should target master. If we want it in PHP-8.0 we also need to back-port 98bdb7f.

static HashTable *exif_make_tag_ht(tag_info_type *tag_table)
{
HashTable *ht = malloc(sizeof(HashTable));
HashTable *ht = pemalloc(sizeof(HashTable), 0);
Copy link
Member

Choose a reason for hiding this comment

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

This should be persistent. ht is later stored in EXIF_G(tag_table_cache) which lives beyond requests, as far as I can see.

if (ip) {
ip = strdup(ip);
cur = ip;
char *dup_ip = estrdup(ip);
Copy link
Member

Choose a reason for hiding this comment

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

From what I can see this functions is called from fpm_php_init_child which is called indirectly from main outside of request context. I guess this should work fine if the allocator is started, I'm not sure which one is preferable.

int ret, len = (int)strlen(message);
char *buf = malloc(len+2);

char *buf = pemalloc(len+2, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this can be called outside of request context.

@Girgias
Copy link
Member

Girgias commented Feb 6, 2023

@hwde Could you please rebase this on top of PHP-8.1 and address review comments?

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.

6 participants