tree-wide: Do not manually check for `NULL` before calling `free()` #19063

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

Jump to bottom

Closed

TimWolla wants to merge 1 commit into php:master from TimWolla:free-null-check

Closed

tree-wide: Do not manually check for `NULL` before calling `free()` #19063

TimWolla wants to merge 1 commit into php:master from TimWolla:free-null-check

+339 −886

Conversation

TimWolla

Copy link

Member

@TimWolla TimWolla commented Jul 7, 2025 •

edited

Loading

Changes performed with Coccinelle:

@@
expression e;
@@
- if (e) {
	efree(e);
- }
@@
expression e;
@@
- if (e) {
	efree(e);
	e = NULL;
- }
@@
expression e;
@@
- if (e) {
	free(e);
- }
@@
expression e;
@@
- if (e) {
	free(e);
	e = NULL;
- }
@@
expression e, e2;
@@
- if (e) {
	pefree(e, e2);
- }
@@
expression e, e2;
@@
- if (e) {
	pefree(e, e2);
	e = NULL;
- }

@TimWolla


 tree-wide: Do not manually check for NULL before calling free()

c4097fa

Changes performed with Coccinelle:
 @@
 expression e;
 @@
 - if (e) {
 	efree(e);
 - }
 @@
 expression e;
 @@
 - if (e) {
 	efree(e);
 	e = NULL;
 - }
 @@
 expression e;
 @@
 - if (e) {
 	free(e);
 - }
 @@
 expression e;
 @@
 - if (e) {
 	free(e);
 	e = NULL;
 - }
 @@
 expression e, e2;
 @@
 - if (e) {
 	pefree(e, e2);
 - }
 @@
 expression e, e2;
 @@
 - if (e) {
 	pefree(e, e2);
 	e = NULL;
 - }

@github-actions github-actions bot added Extension: sqlite3 Extension: standard Extension: xml Extension: zip Extension: zlib Category: Optimizer ABI break labels

Jul 7, 2025

Girgias

Girgias reviewed

Jul 7, 2025

View reviewed changes

Copy link

Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Minor CS nits but looks nice :)

ext/snmp/snmp.c

Comment on lines +150 to 151

efree((*session)->a); \

(*session)->a = NULL; \

Copy link

Member

@Girgias Girgias Jul 7, 2025

Choose a reason for hiding this comment

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

Suggested change

efree((*session)->a); \

(*session)->a = NULL; \

efree((*session)->a); \

(*session)->a = NULL; \

ext/snmp/snmp.c

Comment on lines +363 to +364

/* malloc was used to store value */

efree(dbuf);

Copy link

Member

@Girgias Girgias Jul 7, 2025

Choose a reason for hiding this comment

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

Suggested change

/* malloc was used to store value */

efree(dbuf);

/* malloc was used to store value */

efree(dbuf);

Zend/zend_API.c

Comment on lines 1120 to 1122

efree(error);

}

Copy link

Member

@Girgias Girgias Jul 7, 2025

Choose a reason for hiding this comment

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

Suggested change

efree(error);

}

efree(error);

}

@devnexen

Copy link

Member

devnexen commented Jul 7, 2025

the title caught me :) so not just free but zendmm api flavors.

@TimWolla

Copy link

Member Author

TimWolla commented Jul 10, 2025

Unfortunately this one caused a +0.03% performance regression on the Symfony Benchmark: https://github.com/php/php-src/actions/runs/16120616513?pr=19063

Probably one (or more) of the commonly used structures has a pointer that is mostly NULL, leading to an avoidable call to free().

Nevertheless, I was happy that I could showcase what Coccinelle would be able to provide us and I'm happy to advice for other kinds of cleanups that folks might want to do 😃

@TimWolla TimWolla closed this

Jul 10, 2025

@TimWolla TimWolla deleted the free-null-check branch

July 10, 2025 06:45

Reviewers

@Girgias Girgias Girgias left review comments

@dstogov dstogov Awaiting requested review from dstogov dstogov will be requested when the pull request is marked ready for review dstogov is a code owner

@arnaud-lb arnaud-lb Awaiting requested review from arnaud-lb arnaud-lb will be requested when the pull request is marked ready for review arnaud-lb is a code owner

@nielsdos nielsdos Awaiting requested review from nielsdos nielsdos will be requested when the pull request is marked ready for review nielsdos is a code owner

@bukka bukka Awaiting requested review from bukka bukka will be requested when the pull request is marked ready for review bukka is a code owner

@devnexen devnexen Awaiting requested review from devnexen devnexen will be requested when the pull request is marked ready for review devnexen is a code owner

@SakiTakamachi SakiTakamachi Awaiting requested review from SakiTakamachi SakiTakamachi will be requested when the pull request is marked ready for review SakiTakamachi is a code owner

@NattyNarwhal NattyNarwhal Awaiting requested review from NattyNarwhal NattyNarwhal will be requested when the pull request is marked ready for review NattyNarwhal is a code owner

@kamil-tekiela kamil-tekiela Awaiting requested review from kamil-tekiela kamil-tekiela will be requested when the pull request is marked ready for review kamil-tekiela is a code owner

@alexdowad alexdowad Awaiting requested review from alexdowad alexdowad will be requested when the pull request is marked ready for review alexdowad is a code owner

@youkidearitai youkidearitai Awaiting requested review from youkidearitai youkidearitai will be requested when the pull request is marked ready for review youkidearitai is a code owner

@derickr derickr Awaiting requested review from derickr derickr will be requested when the pull request is marked ready for review derickr is a code owner

Labels

ABI break Category: Engine Category: Optimizer Extension: calendar Extension: com_dotnet Extension: date Extension: dba Extension: fileinfo Extension: ftp Extension: gd Extension: hash Extension: intl Extension: mbstring Extension: mysqli Extension: mysqlnd Extension: odbc Extension: opcache Extension: openssl Extension: pdo (core) Extension: pdo_dblib Extension: pdo_firebird Extension: pdo_mysql Extension: pdo_odbc Extension: pdo_pgsql Extension: pdo_sqlite Extension: pgsql Extension: phar Extension: readline Extension: snmp Extension: soap Extension: sockets Extension: sqlite3 Extension: standard Extension: xml Extension: zip Extension: zlib

3 participants

@TimWolla @devnexen @Girgias

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

tree-wide: Do not manually check for `NULL` before calling `free()` #19063

tree-wide: Do not manually check for `NULL` before calling `free()` #19063

Conversation

@TimWolla TimWolla commented Jul 7, 2025 •

edited

Loading

Uh oh!

Uh oh!

@Girgias Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

@Girgias Girgias Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

@Girgias Girgias Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

@Girgias Girgias Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

devnexen commented Jul 7, 2025

Uh oh!

TimWolla commented Jul 10, 2025

Uh oh!

Uh oh!

tree-wide: Do not manually check for NULL before calling free() #19063

tree-wide: Do not manually check for NULL before calling free() #19063

Conversation

@TimWolla TimWolla commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@Girgias Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

@Girgias Girgias Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

@Girgias Girgias Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

@Girgias Girgias Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

devnexen commented Jul 7, 2025

Uh oh!

TimWolla commented Jul 10, 2025

Uh oh!

Uh oh!

tree-wide: Do not manually check for `NULL` before calling `free()` #19063

tree-wide: Do not manually check for `NULL` before calling `free()` #19063

@TimWolla TimWolla commented Jul 7, 2025 •

edited

Loading