Note where a session was already started (#10736)

* Note where a session was already started

Duplicated session starts can be annoying to debug. The error that
occurs when a session is already active doesn't tell you where it
was initialized, so figuring out the callsite involves manual
debugging to find it out.

This keeps track of the call site of session_start as a request
global, and frees at the end of the request. It should make it
easier to find these instances for PHP users.

The resulting message can look like:
Notice: session_start(): Ignoring session_start() because a session is already active (started from /home/calvin/src/php-src/inc.php on line 4) in /home/calvin/src/php-src/index.php on line 9

Fixes GH-10721

* Convert to using zend_string for session start location

* Fix leak with session start callsite filename

If this was already initialized, we'd forget it. Have shared free
between session_start and RSHUTDOWN.

* For sessions that are automatically started, note that

Easy to forget that you have this set, in which case, session start
is done at RINIT outside of user code. Because this config option
can't change at runtime, we can check for it and make the error
more specific if that's the case.

Author: NattyNarwhal

ext/session/php_session.h

@@ -154,6 +154,8 @@ typedef struct _php_ps_globals {
 	const ps_module *default_mod;
 	void *mod_data;
 	php_session_status session_status;
+	zend_string *session_started_filename;
+	uint32_t session_started_lineno;
 	zend_long gc_probability;
 	zend_long gc_divisor;
 	zend_long gc_maxlifetime;

ext/session/session.c

@@ -121,6 +121,16 @@ static inline void php_rinit_session_globals(void) /* {{{ */
 }
 /* }}} */
 
+static inline void php_session_cleanup_filename(void) /* {{{ */
+{
+	if (PS(session_started_filename)) {
+		zend_string_release(PS(session_started_filename));
+		PS(session_started_filename) = NULL;
+		PS(session_started_lineno) = 0;
+	}
+}
+/* }}} */
+
 /* Dispatched by RSHUTDOWN and by php_session_destroy */
 static inline void php_rshutdown_session_globals(void) /* {{{ */
 {
@@ -149,6 +159,8 @@ static inline void php_rshutdown_session_globals(void) /* {{{ */
 		PS(mod_user_class_name) = NULL;
 	}
 
+	php_session_cleanup_filename();
+
 	/* User save handlers may end up directly here by misuse, bugs in user script, etc. */
 	/* Set session status to prevent error while restoring save handler INI value. */
 	PS(session_status) = php_session_none;
@@ -465,6 +477,13 @@ static zend_result php_session_initialize(void) /* {{{ */
 		php_session_decode(val);
 		zend_string_release_ex(val, 0);
 	}
+
+	php_session_cleanup_filename();
+	zend_string *session_started_filename = zend_get_executed_filename_ex();
+	if (session_started_filename != NULL) {
+		PS(session_started_filename) = zend_string_copy(session_started_filename);
+		PS(session_started_lineno) = zend_get_executed_lineno();
+	}
 	return SUCCESS;
 }
 /* }}} */
@@ -1490,7 +1509,14 @@ PHPAPI zend_result php_session_start(void) /* {{{ */
 
 	switch (PS(session_status)) {
 		case php_session_active:
-			php_error(E_NOTICE, "Ignoring session_start() because a session has already been started");
+			if (PS(session_started_filename)) {
+				php_error(E_NOTICE, "Ignoring session_start() because a session has already been started (started from %s on line %"PRIu32")", ZSTR_VAL(PS(session_started_filename)), PS(session_started_lineno));
+			} else if (PS(auto_start)) {
+				/* This option can't be changed at runtime, so we can assume it's because of this */
+				php_error(E_NOTICE, "Ignoring session_start() because a session has already been started automatically");
+			} else {
+				php_error(E_NOTICE, "Ignoring session_start() because a session has already been started");
+			}
 			return FAILURE;
 			break;
 
@@ -1600,6 +1626,7 @@ PHPAPI zend_result php_session_start(void) /* {{{ */
 		}
 		return FAILURE;
 	}
+
 	return SUCCESS;
 }
 /* }}} */
@@ -2513,7 +2540,14 @@ PHP_FUNCTION(session_start)
 	}
 
 	if (PS(session_status) == php_session_active) {
-		php_error_docref(NULL, E_NOTICE, "Ignoring session_start() because a session is already active");
+		if (PS(session_started_filename)) {
+			php_error_docref(NULL, E_NOTICE, "Ignoring session_start() because a session is already active (started from %s on line %"PRIu32")", ZSTR_VAL(PS(session_started_filename)), PS(session_started_lineno));
+		} else if (PS(auto_start)) {
+			/* This option can't be changed at runtime, so we can assume it's because of this */
+			php_error_docref(NULL, E_NOTICE, "Ignoring session_start() because a session is already automatically active");
+		} else {
+			php_error_docref(NULL, E_NOTICE, "Ignoring session_start() because a session is already active");
+		}
 		RETURN_TRUE;
 	}
 
@@ -2846,6 +2880,8 @@ static PHP_MINIT_FUNCTION(session) /* {{{ */
 	PS(module_number) = module_number;
 
 	PS(session_status) = php_session_none;
+	PS(session_started_filename) = NULL;
+	PS(session_started_lineno) = 0;
 	REGISTER_INI_ENTRIES();
 
 #ifdef HAVE_LIBMM

ext/session/tests/session_start_variation1.phpt

@@ -25,15 +25,15 @@ ob_end_flush();
 *** Testing session_start() : variation ***
 bool(true)
 
-Notice: session_start(): Ignoring session_start() because a session is already active in %s on line %d
+Notice: session_start(): Ignoring session_start() because a session is already active (started from %s on line %d) in %s on line %d
 bool(true)
 
-Notice: session_start(): Ignoring session_start() because a session is already active in %s on line %d
+Notice: session_start(): Ignoring session_start() because a session is already active (started from %s on line %d) in %s on line %d
 bool(true)
 
-Notice: session_start(): Ignoring session_start() because a session is already active in %s on line %d
+Notice: session_start(): Ignoring session_start() because a session is already active (started from %s on line %d) in %s on line %d
 bool(true)
 
-Notice: session_start(): Ignoring session_start() because a session is already active in %s on line %d
+Notice: session_start(): Ignoring session_start() because a session is already active (started from %s on line %d) in %s on line %d
 bool(true)
 Done

ext/session/tests/session_start_variation9.phpt

@@ -26,7 +26,7 @@ ob_end_flush();
 *** Testing session_start() : variation ***
 string(%d) "%s"
 
-Notice: session_start(): Ignoring session_start() because a session is already active in %s on line %d
+Notice: session_start(): Ignoring session_start() because a session is already automatically active in %s on line %d
 bool(true)
 string(%d) "%s"
 bool(true)