ext/session refactoring: INI settings #16419

jorgsowa · 2024-10-13T17:37:53Z

Rebase of PR: #9322

Girgias

Thanks for tackling this again! I still don't understand why I needed to duplicate some of the strings, maybe @nielsdos has an idea?

ext/session/session.c

Girgias · 2024-10-14T10:58:33Z

ext/session/session.c

+		return false;
+	}
+	/* Prevent broken Set-Cookie header, because the session_name might be user supplied */
+	if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\013\014") != NULL) {   /* man isspace for \013 and \014 */


I know this is an existing issue but \013 is a vertical tab (\v) and \014 is a form feed (\f) which would probably be clearer than needed to look up some documentation

Girgias · 2024-10-14T11:00:57Z

ext/session/session.c

+		// TODO Use zend_string_cmp API?
+		if (!strcasecmp(lim->name, ZSTR_VAL(PS(cache_limiter)))) {


Annoyingly we don't have a zend_string_equals_cstr_ci() API.

Girgias · 2024-10-14T11:01:42Z

ext/session/session.c

+	spprintf(&session_cookie, 0, "Set-Cookie: %s=", ZSTR_VAL(PS(session_name)));

+	// TODO Manually compute from known information?
 	session_cookie_len = strlen(session_cookie);


Maybe using the strpprintf API to generate a zend_string is better here?

Generate a zend_string here.

nielsdos · 2024-10-14T17:35:00Z

I still don't understand why I needed to duplicate some of the strings, maybe @nielsdos has an idea?

@Girgias This is because the strings are persistent, but don't have GC_PERSISTENT_LOCAL set. If you're on a multithreaded SAPI/ZTS, then sharing the same non-interned persistent strings can cause race conditions when changing the refcounts. If GC_PERSISTENT_LOCAL is set this check is skipped because you're saying to the engine that these strings are local to the current thread, and thus the reference counts cannot be racy. (Make sure you only do this if you're certain that these strings are not shared across threads). In this particular case, the strings come from INI settings, and I haven't checked if they are local to the current thread, but my gut feeling says they're shared across threads.

Girgias · 2024-10-15T11:06:56Z

I still don't understand why I needed to duplicate some of the strings, maybe @nielsdos has an idea?

@Girgias This is because the strings are persistent, but don't have GC_PERSISTENT_LOCAL set. If you're on a multithreaded SAPI/ZTS, then sharing the same non-interned persistent strings can cause race conditions when changing the refcounts. If GC_PERSISTENT_LOCAL is set this check is skipped because you're saying to the engine that these strings are local to the current thread, and thus the reference counts cannot be racy. (Make sure you only do this if you're certain that these strings are not shared across threads). In this particular case, the strings come from INI settings, and I haven't checked if they are local to the current thread, but my gut feeling says they're shared across threads.

Hum, I suppose the default value of the INI setting might be shared across threads, but I don't know about overwritten values 🤔

However, the code is somewhat disgusting as I'm hitting Zend/zend_types.h:1222: zend_gc_delref: Assertion (zval_gc_flags((p)->u.type_info) & ((1<<7)|(1<<8))) != (1<<7) failed. Failures that I don't understand.

Also fixed a bug where nul bytes where not properly checked

Co-authored-by: Tim Düsterhus <timwolla@googlemail.com>

jorgsowa · 2025-01-10T19:17:24Z

I fixed failing tests on ZTS by changing OnUpdateString to

	zend_string **p = (zend_string **) ZEND_INI_GET_ADDR();
	*p = new_value ? new_value : NULL;
	return SUCCESS;

I'm not quite sure why it was failing though. Anyways, now everything seems to pass.

Girgias

Do we still need the duplication?

Girgias · 2025-01-13T11:52:03Z

ext/session/session.c

+	if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\v\f") != NULL) {   /* man isspace for \v and \f */
+		if (diagnostic_type) {
+			php_error_docref(NULL, diagnostic_type, "session.name \"%s\" must not contain any of the following "
+				"'=,; \\t\\r\\n\\v\\f'", ZSTR_VAL(name));
+		}
+		return false;


Suggested change

if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\v\f") != NULL) { /* man isspace for \v and \f */

if (diagnostic_type) {

php_error_docref(NULL, diagnostic_type, "session.name \"%s\" must not contain any of the following "

"'=,; \\t\\r\\n\\v\\f'", ZSTR_VAL(name));

}

return false;

if (strpbrk(ZSTR_VAL(name), SESSION_FORBIDDEN_CHARS) != NULL) { /* man isspace for \v and \f */

if (diagnostic_type) {

php_error_docref(NULL, diagnostic_type, "session.name \"%s\" must not contain any of the following \""

SESSION_FORBIDDEN_CHARS "\"", ZSTR_VAL(name));

}

return false;

jorgsowa requested a review from Girgias as a code owner October 13, 2024 17:37

github-actions bot added Extension: session ABI break labels Oct 13, 2024

Girgias reviewed Oct 14, 2024

View reviewed changes

jorgsowa force-pushed the session-ext-improvements branch from db5e63a to 07dbb54 Compare December 6, 2024 00:10

Girgias and others added 10 commits January 10, 2025 19:45

Convert some Session globals from char* to zend_string*

b60e2f7

However, the code is somewhat disgusting as I'm hitting Zend/zend_types.h:1222: zend_gc_delref: Assertion (zval_gc_flags((p)->u.type_info) & ((1<<7)|(1<<8))) != (1<<7) failed. Failures that I don't understand.

Convert session_name from char to zend_string

7c04d83

Also fixed a bug where nul bytes where not properly checked

Use NUL spelling

b1c18ae

Co-authored-by: Tim Düsterhus <timwolla@googlemail.com>

address review comments

3b213dd

Check for session name criterias consistently

b03b233

Use new API for checking if string has NUL bytes

b01aa3e

Use known length

aaa67ee

Rebase alignments

0393e65

Improved error messages

6660a1c

Replaced invalid OnUpdateString

d5de6ed

jorgsowa force-pushed the session-ext-improvements branch from 07dbb54 to d5de6ed Compare January 10, 2025 18:45

jorgsowa requested a review from Girgias January 10, 2025 19:17

Girgias requested changes Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ext/session refactoring: INI settings #16419

ext/session refactoring: INI settings #16419

Uh oh!

jorgsowa commented Oct 13, 2024

Uh oh!

Girgias left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Girgias Oct 14, 2024

Uh oh!

Girgias Oct 14, 2024

Uh oh!

Girgias Oct 14, 2024

Uh oh!

Girgias Jan 13, 2025

Uh oh!

nielsdos commented Oct 14, 2024 •

edited

Loading

Uh oh!

Girgias commented Oct 15, 2024

Uh oh!

jorgsowa commented Jan 10, 2025

Uh oh!

Girgias left a comment

Uh oh!

Girgias Jan 13, 2025

Uh oh!

Uh oh!

		// TODO Use zend_string_cmp API?
		if (!strcasecmp(lim->name, ZSTR_VAL(PS(cache_limiter)))) {

ext/session refactoring: INI settings #16419

Are you sure you want to change the base?

ext/session refactoring: INI settings #16419

Uh oh!

Conversation

jorgsowa commented Oct 13, 2024

Uh oh!

Girgias left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Girgias Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

Girgias Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

Girgias Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

Girgias Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

nielsdos commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Girgias commented Oct 15, 2024

Uh oh!

jorgsowa commented Jan 10, 2025

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Girgias Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Girgias left a comment •

edited

Loading

nielsdos commented Oct 14, 2024 •

edited

Loading