Skip to content

Conversation

@SakiTakamachi
Copy link
Member

@SakiTakamachi SakiTakamachi commented Apr 8, 2025

fixes #18276

The structure has changed slightly on master, so I don’t plan to apply the change the third commit to master.
I’ll follow up with a separate PR for changes targeting master.

@SakiTakamachi SakiTakamachi force-pushed the fix/gh18276_3 branch 3 times, most recently from e175d97 to bd8ad0e Compare April 8, 2025 14:32
@SakiTakamachi
Copy link
Member Author

Ah, the cause appears to be a mixture of persistent and non-persistent memory.

It's too late now so I'll fix it tomorrow.

@SakiTakamachi SakiTakamachi marked this pull request as ready for review April 9, 2025 01:22
@SakiTakamachi SakiTakamachi requested a review from nielsdos April 9, 2025 01:22
if (H->date_format) {
efree(H->date_format);
pefree(H->date_format, dbh->is_persistent);
H->date_format = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Since you're freeing the entire structure anyway below, you don't need to set the pointers for these 3 fields to NULL.

if (H->date_format) {
efree(H->date_format);
pefree(H->date_format, dbh->is_persistent);
H->date_format = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with these being set to NULL because technically we could bail out if the pestrdup below fails.

H->date_format = NULL;
}
spprintf(&H->date_format, 0, "%s", ZSTR_VAL(str));
H->date_format = pestrdup(ZSTR_VAL(str), dbh->is_persistent);
Copy link
Member

Choose a reason for hiding this comment

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

Since we know the length, you should use pestrndup instead and pass in ZSTR_LEN(str) as well (same below).

PDO::ATTR_PERSISTENT => true,
],
);
// Avoid interned
Copy link
Member

Choose a reason for hiding this comment

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

str_repeat is special cased in the SCCP optimization pass if you have a constant number as a second argument, and so you'll still get an interned string here.
You need to do this: str_repeat('my string', random_int(1, 1));

@SakiTakamachi
Copy link
Member Author

@nielsdos
Fixed :)

SakiTakamachi added a commit to SakiTakamachi/php-src that referenced this pull request Apr 11, 2025
SakiTakamachi added a commit to SakiTakamachi/php-src that referenced this pull request Apr 11, 2025
SakiTakamachi added a commit that referenced this pull request Apr 15, 2025
* PHP-8.3:
  Fixed GH-18276 - persistent connection - "zend_mm_heap corrupted" with setAttribute() (#18280) Closes #18280 Fixes #18276
SakiTakamachi added a commit that referenced this pull request Apr 15, 2025
SakiTakamachi added a commit that referenced this pull request Apr 15, 2025
* PHP-8.4:
  Fixed GH-18276 - persistent connection - "zend_mm_heap corrupted" with setAttribute() (#18280) Closes #18280 Fixes #18276
@SakiTakamachi SakiTakamachi deleted the fix/gh18276_3 branch April 15, 2025 00:07
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.

2 participants