Skip to content

Commit 64fb267

Browse files
kocsismatekocsismate
committed
Address review comments
1 parent c74573f commit 64fb267

File tree

7 files changed

+105
-28
lines changed

7 files changed

+105
-28
lines changed

‎ext/session/mod_files.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ static void ps_files_open(ps_files *data, const char *key)
168168
ps_files_close(data);
169169

170170
if (php_session_valid_key(key) == FAILURE) {
171-
php_error_docref(NULL, E_WARNING, "Session ID is too long or contains illegal characters. Valid characters are a-z, A-Z, 0-9, and \"-\"");
171+
php_error_docref(NULL, E_WARNING, "Session ID is too long or contains illegal characters. Only the A-Z, a-z, 0-9, \"-\", and \",\" characters are allowed");
172172
return;
173173
}
174174

‎ext/session/mod_user.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ static void ps_call_handler(zval *func, int argc, zval *argv, zval *retval)
6969
ret = SUCCESS; \
7070
} else { \
7171
if (!EG(exception)) { \
72-
php_error_docref(NULL, E_WARNING, "Session callback must have a return value of type bool, %s returned", zend_zval_type_name(&retval)); \
72+
zend_type_error("Session callback must have a return value of type bool, %s returned", zend_zval_type_name(&retval)); \
7373
} \
7474
ret = FAILURE; \
7575
zval_ptr_dtor(&retval); \

‎ext/session/session.c

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2051,13 +2051,6 @@ PHP_FUNCTION(session_set_save_handler)
20512051
RETURN_THROWS();
20522052
}
20532053

2054-
if (save_handler_check_session() == FAILURE) {
2055-
RETURN_FALSE;
2056-
}
2057-
2058-
/* remove shutdown function */
2059-
remove_user_shutdown_function("session_shutdown", sizeof("session_shutdown") - 1);
2060-
20612054
/* At this point argc can only be between 6 and PS_NUM_APIS */
20622055
for (i = 0; i < argc; i++) {
20632056
if (!zend_is_callable(&args[i], 0, NULL)) {
@@ -2068,6 +2061,13 @@ PHP_FUNCTION(session_set_save_handler)
20682061
}
20692062
}
20702063

2064+
if (save_handler_check_session() == FAILURE) {
2065+
RETURN_FALSE;
2066+
}
2067+
2068+
/* remove shutdown function */
2069+
remove_user_shutdown_function("session_shutdown", sizeof("session_shutdown") - 1);
2070+
20712071
if (PS(mod) && PS(mod) != &ps_mod_user) {
20722072
ini_name = zend_string_init("session.save_handler", sizeof("session.save_handler") - 1, 0);
20732073
ini_val = zend_string_init("user", sizeof("user") - 1, 0);
@@ -2274,7 +2274,7 @@ PHP_FUNCTION(session_create_id)
22742274
if (prefix && ZSTR_LEN(prefix)) {
22752275
if (php_session_valid_key(ZSTR_VAL(prefix)) == FAILURE) {
22762276
/* E_ERROR raised for security reason. */
2277-
php_error_docref(NULL, E_WARNING, "Prefix cannot contain special characters. Only alphanumeric and \",\", \"-\" characters are allowed");
2277+
php_error_docref(NULL, E_WARNING, "Prefix cannot contain special characters. Only the A-Z, a-z, 0-9, \"-\", and \",\" characters are allowed");
22782278
RETURN_FALSE;
22792279
} else {
22802280
smart_str_append(&id, prefix);
@@ -2427,7 +2427,7 @@ static int php_session_start_set_ini(zend_string *varname, zend_string *new_valu
24272427
return ret;
24282428
}
24292429

2430-
/* {{{ + Begin session */
2430+
/* {{{ Begin session */
24312431
PHP_FUNCTION(session_start)
24322432
{
24332433
zval *options = NULL;
@@ -2470,7 +2470,7 @@ PHP_FUNCTION(session_start)
24702470
zend_string *tmp_val;
24712471
zend_string *val = zval_get_tmp_string(value, &tmp_val);
24722472
if (php_session_start_set_ini(str_idx, val) == FAILURE) {
2473-
php_error_docref(NULL, E_WARNING, "Option \"%s\" cannot be set", ZSTR_VAL(str_idx));
2473+
php_error_docref(NULL, E_WARNING, "Setting option \"%s\" failed", ZSTR_VAL(str_idx));
24742474
}
24752475
zend_tmp_string_release(tmp_val);
24762476
}
@@ -2479,7 +2479,7 @@ PHP_FUNCTION(session_start)
24792479
zend_type_error("%s(): Option \"%s\" must be of type string|int|bool, %s given",
24802480
get_active_function_name(), ZSTR_VAL(str_idx), zend_zval_type_name(value)
24812481
);
2482-
break;
2482+
RETURN_THROWS();
24832483
}
24842484
}
24852485
(void) num_idx;

‎ext/session/tests/rfc1867_sid_invalid.phpt

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,14 +45,18 @@ var_dump($_FILES);
4545
var_dump($_SESSION["upload_progress_" . basename(__FILE__)]);
4646
session_destroy();
4747
?>
48+
--CLEAN--
49+
<?php
50+
@unlink(__DIR__ . DIRECTORY_SEPARATOR . "rfc1867_sid_invalid.post.txt");
51+
?>
4852
--EXPECTF--
49-
Warning: Unknown: Session ID is too long or contains illegal characters. Valid characters are a-z, A-Z, 0-9, and "-" in Unknown on line 0
53+
Warning: Unknown: Session ID is too long or contains illegal characters. Only the A-Z, a-z, 0-9, "-", and "," characters are allowed in Unknown on line 0
5054

5155
Warning: Unknown: Failed to read session data: files (path: ) in Unknown on line 0
5256

5357
Warning: Unknown: Failed to write session data (files). Please verify that the current setting of session.save_path is correct () in Unknown on line 0
5458

55-
Warning: Unknown: Session ID is too long or contains illegal characters. Valid characters are a-z, A-Z, 0-9, and "-" in Unknown on line 0
59+
Warning: Unknown: Session ID is too long or contains illegal characters. Only the A-Z, a-z, 0-9, "-", and "," characters are allowed in Unknown on line 0
5660

5761
Warning: Unknown: Failed to read session data: files (path: ) in Unknown on line 0
5862

‎ext/session/tests/rfc1867_sid_invalid.post

Lines changed: 0 additions & 13 deletions
This file was deleted.

‎ext/session/tests/session_set_save_handler_type_error.phpt

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
--TEST--
2+
Test session_set_save_handler() function: interface wrong
3+
--SKIPIF--
4+
<?php include('skipif.inc'); ?>
5+
--INI--
6+
session.name=PHPSESSID
7+
session.save_handler=files
8+
--FILE--
9+
<?php
10+
11+
$validCallback = function () { return true; };
12+
$deprecatedCallback = function () { return 0; };
13+
$exceptionCallback = function () { return []; };
14+
15+
ob_start();
16+
17+
try {
18+
$ret = session_set_save_handler($exceptionCallback, $validCallback, $validCallback, $validCallback, $validCallback, $validCallback);
19+
session_start();
20+
} catch (TypeError $exception) {
21+
echo $exception->getMessage() . "\n";
22+
}
23+
24+
try {
25+
$ret = session_set_save_handler($deprecatedCallback, $validCallback, $validCallback, $validCallback, $validCallback, $validCallback);
26+
session_start();
27+
} catch (TypeError $exception) {
28+
echo $exception->getMessage() . "\n";
29+
}
30+
31+
try {
32+
$ret = session_set_save_handler($validCallback, $exceptionCallback, $validCallback, $validCallback, $validCallback, $validCallback);
33+
session_start();
34+
} catch (TypeError $exception) {
35+
echo $exception->getMessage() . "\n";
36+
}
37+
38+
try {
39+
$ret = session_set_save_handler($validCallback, $deprecatedCallback, $exceptionCallback, $validCallback, $validCallback, $validCallback);
40+
session_start();
41+
} catch (TypeError $exception) {
42+
echo $exception->getMessage() . "\n";
43+
}
44+
45+
ob_end_flush();
46+
47+
?>
48+
--EXPECTF--
49+
Warning: session_start(): Failed to initialize storage module: user (path: ) in %s on line %d
50+
Session callback must have a return value of type bool, array returned
51+
52+
Deprecated: session_start(): Session callback must have a return value of type bool, int returned in %s on line %d
53+
54+
Warning: session_start(): Failed to read session data: user (%s) in %s on line %d
55+
56+
Warning: session_start(): Failed to read session data: user (path: ) in %s on line %d
57+
Session callback must have a return value of type bool, array returned
58+
59+
Deprecated: session_start(): Session callback must have a return value of type bool, int returned in %s on line %d
60+
61+
Warning: session_start(): Failed to read session data: user (%s) in %s on line %d

‎ext/session/tests/session_start_error.phpt

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
--TEST--
2+
Test session_start() errors
3+
--SKIPIF--
4+
<?php include('skipif.inc'); ?>
5+
--FILE--
6+
<?php
7+
8+
ob_start();
9+
10+
try {
11+
session_start(['option' => new stdClass()]);
12+
} catch (TypeError $exception) {
13+
echo $exception->getMessage() . "\n";
14+
}
15+
16+
var_dump(session_start(['option' => false]));
17+
18+
ob_end_flush();
19+
20+
?>
21+
--EXPECTF--
22+
session_start(): Option "option" must be of type string|int|bool, stdClass given
23+
24+
Warning: session_start(): Setting option "option" failed in %s on line %d
25+
bool(true)

0 commit comments

Comments
 (0)