From d3d8859690eb249d35d87e158333c31e7e431c98 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 5 Jan 2025 15:25:29 +0000 Subject: [PATCH 01/10] ext/snmp: various internals rewrite. --- ext/snmp/snmp.c | 83 ++++++++++++++++++++++++++++++------------------- 1 file changed, 51 insertions(+), 32 deletions(-) diff --git a/ext/snmp/snmp.c b/ext/snmp/snmp.c index 91b530f3b39d7..1fbb60b91b79d 100644 --- a/ext/snmp/snmp.c +++ b/ext/snmp/snmp.c @@ -153,7 +153,7 @@ static PHP_GINIT_FUNCTION(snmp) } \ } -static void netsnmp_session_free(php_snmp_session **session) /* {{{ */ +static void snmp_session_free(php_snmp_session **session) /* {{{ */ { if (*session) { PHP_SNMP_SESSION_FREE(peername); @@ -174,7 +174,7 @@ static void php_snmp_object_free_storage(zend_object *object) /* {{{ */ return; } - netsnmp_session_free(&(intern->session)); + snmp_session_free(&(intern->session)); zend_object_std_dtor(&intern->zo); } @@ -829,10 +829,10 @@ static bool php_snmp_parse_oid( } /* }}} */ -/* {{{ netsnmp_session_init - allocates memory for session and session->peername, caller should free it manually using netsnmp_session_free() and efree() +/* {{{ snmp_session_init + allocates memory for session and session->peername, caller should free it manually using session_free() and efree() */ -static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, int timeout, int retries) +static bool snmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, int timeout, int retries) { php_snmp_session *session; char *pptr, *host_ptr; @@ -840,8 +840,15 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend int n; struct sockaddr **psal; struct sockaddr **res; + + if (ZSTR_LEN(hostname) >= MAX_NAME_LEN) { + php_error_docref(NULL, E_WARNING, "hostname length must be lower than %d", MAX_NAME_LEN); + return false; + } + // TODO: Do not strip and re-add the port in peername? - unsigned remote_port = SNMP_PORT; + unsigned short remote_port = SNMP_PORT; + int tmp_port; *session_p = (php_snmp_session *)emalloc(sizeof(php_snmp_session)); session = *session_p; @@ -853,7 +860,7 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend session->peername = emalloc(MAX_NAME_LEN); /* we copy original hostname for further processing */ - strlcpy(session->peername, ZSTR_VAL(hostname), MAX_NAME_LEN); + memcpy(session->peername, ZSTR_VAL(hostname), ZSTR_LEN(hostname) + 1); host_ptr = session->peername; /* Reading the hostname and its optional non-default port number */ @@ -862,7 +869,13 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend host_ptr++; if ((pptr = strchr(host_ptr, ']'))) { if (pptr[1] == ':') { - remote_port = atoi(pptr + 2); + char *pport = pptr + 2; + tmp_port = atoi(pport); + if (tmp_port < 0 || tmp_port > USHRT_MAX) { + php_error_docref(NULL, E_WARNING, "Remote port must be between 0 and %u", USHRT_MAX); + return false; + } + remote_port = (unsigned short)tmp_port; } *pptr = '\0'; } else { @@ -871,7 +884,13 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend } } else { /* IPv4 address */ if ((pptr = strchr(host_ptr, ':'))) { - remote_port = atoi(pptr + 1); + char *pport = pptr + 1; + tmp_port = atoi(pport); + if (tmp_port < 0 || tmp_port > USHRT_MAX) { + php_error_docref(NULL, E_WARNING, "Remote port must be between 0 and %u", USHRT_MAX); + return false; + } + remote_port = (unsigned short)tmp_port; *pptr = '\0'; } } @@ -920,7 +939,7 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend if (remote_port != SNMP_PORT) { size_t peername_length = strlen(session->peername); pptr = session->peername + peername_length; - snprintf(pptr, MAX_NAME_LEN - peername_length, ":%d", remote_port); + snprintf(pptr, MAX_NAME_LEN - peername_length, ":%u", remote_port); } php_network_freeaddresses(psal); @@ -942,7 +961,7 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend /* }}} */ /* {{{ Set the security level in the snmpv3 session */ -static bool netsnmp_session_set_sec_level(struct snmp_session *s, zend_string *level) +static bool snmp_session_set_sec_level(struct snmp_session *s, zend_string *level) { if (zend_string_equals_literal_ci(level, "noAuthNoPriv") || zend_string_equals_literal_ci(level, "nanp")) { s->securityLevel = SNMP_SEC_LEVEL_NOAUTH; @@ -959,7 +978,7 @@ static bool netsnmp_session_set_sec_level(struct snmp_session *s, zend_string *l /* }}} */ /* {{{ Set the authentication protocol in the snmpv3 session */ -static bool netsnmp_session_set_auth_protocol(struct snmp_session *s, zend_string *prot) +static bool snmp_session_set_auth_protocol(struct snmp_session *s, zend_string *prot) { #ifndef DISABLE_MD5 if (zend_string_equals_literal_ci(prot, "MD5")) { @@ -1011,7 +1030,7 @@ static bool netsnmp_session_set_auth_protocol(struct snmp_session *s, zend_strin /* }}} */ /* {{{ Set the security protocol in the snmpv3 session */ -static bool netsnmp_session_set_sec_protocol(struct snmp_session *s, zend_string *prot) +static bool snmp_session_set_sec_protocol(struct snmp_session *s, zend_string *prot) { #ifndef NETSNMP_DISABLE_DES if (zend_string_equals_literal_ci(prot, "DES")) { @@ -1048,7 +1067,7 @@ static bool netsnmp_session_set_sec_protocol(struct snmp_session *s, zend_string /* }}} */ /* {{{ Make key from pass phrase in the snmpv3 session */ -static bool netsnmp_session_gen_auth_key(struct snmp_session *s, zend_string *pass) +static bool snmp_session_gen_auth_key(struct snmp_session *s, zend_string *pass) { int snmp_errno; s->securityAuthKeyLen = USM_AUTH_KU_LEN; @@ -1063,7 +1082,7 @@ static bool netsnmp_session_gen_auth_key(struct snmp_session *s, zend_string *pa /* }}} */ /* {{{ Make key from pass phrase in the snmpv3 session */ -static bool netsnmp_session_gen_sec_key(struct snmp_session *s, zend_string *pass) +static bool snmp_session_gen_sec_key(struct snmp_session *s, zend_string *pass) { int snmp_errno; @@ -1079,7 +1098,7 @@ static bool netsnmp_session_gen_sec_key(struct snmp_session *s, zend_string *pas /* }}} */ /* {{{ Set context Engine Id in the snmpv3 session */ -static bool netsnmp_session_set_contextEngineID(struct snmp_session *s, zend_string * contextEngineID) +static bool snmp_session_set_contextEngineID(struct snmp_session *s, zend_string * contextEngineID) { size_t ebuf_len = 32, eout_len = 0; uint8_t *ebuf = (uint8_t *) emalloc(ebuf_len); @@ -1102,13 +1121,13 @@ static bool netsnmp_session_set_contextEngineID(struct snmp_session *s, zend_str /* }}} */ /* {{{ Set all snmpv3-related security options */ -static bool netsnmp_session_set_security(struct snmp_session *session, zend_string *sec_level, +static bool snmp_session_set_security(struct snmp_session *session, zend_string *sec_level, zend_string *auth_protocol, zend_string *auth_passphrase, zend_string *priv_protocol, zend_string *priv_passphrase, zend_string *contextName, zend_string *contextEngineID) { /* Setting the security level. */ - if (!netsnmp_session_set_sec_level(session, sec_level)) { + if (!snmp_session_set_sec_level(session, sec_level)) { /* ValueError already generated, just bail out */ return false; } @@ -1116,26 +1135,26 @@ static bool netsnmp_session_set_security(struct snmp_session *session, zend_stri if (session->securityLevel == SNMP_SEC_LEVEL_AUTHNOPRIV || session->securityLevel == SNMP_SEC_LEVEL_AUTHPRIV) { /* Setting the authentication protocol. */ - if (!netsnmp_session_set_auth_protocol(session, auth_protocol)) { + if (!snmp_session_set_auth_protocol(session, auth_protocol)) { /* ValueError already generated, just bail out */ return false; } /* Setting the authentication passphrase. */ - if (!netsnmp_session_gen_auth_key(session, auth_passphrase)) { + if (!snmp_session_gen_auth_key(session, auth_passphrase)) { /* Warning message sent already, just bail out */ return false; } if (session->securityLevel == SNMP_SEC_LEVEL_AUTHPRIV) { /* Setting the security protocol. */ - if (!netsnmp_session_set_sec_protocol(session, priv_protocol)) { + if (!snmp_session_set_sec_protocol(session, priv_protocol)) { /* ValueError already generated, just bail out */ return false; } /* Setting the security protocol passphrase. */ - if (!netsnmp_session_gen_sec_key(session, priv_passphrase)) { + if (!snmp_session_gen_sec_key(session, priv_passphrase)) { /* Warning message sent already, just bail out */ return false; } @@ -1149,7 +1168,7 @@ static bool netsnmp_session_set_security(struct snmp_session *session, zend_stri } /* Setting contextEngineIS if specified */ - if (contextEngineID && ZSTR_LEN(contextEngineID) && !netsnmp_session_set_contextEngineID(session, contextEngineID)) { + if (contextEngineID && ZSTR_LEN(contextEngineID) && !snmp_session_set_contextEngineID(session, contextEngineID)) { /* Warning message sent already, just bail out */ return false; } @@ -1289,14 +1308,14 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) } if (session_less_mode) { - if (!netsnmp_session_init(&session, version, a1, a2, timeout, retries)) { + if (!snmp_session_init(&session, version, a1, a2, timeout, retries)) { php_free_objid_query(&objid_query, oid_ht, value_ht, st); - netsnmp_session_free(&session); + snmp_session_free(&session); RETURN_FALSE; } - if (version == SNMP_VERSION_3 && !netsnmp_session_set_security(session, a3, a4, a5, a6, a7, NULL, NULL)) { + if (version == SNMP_VERSION_3 && !snmp_session_set_security(session, a3, a4, a5, a6, a7, NULL, NULL)) { php_free_objid_query(&objid_query, oid_ht, value_ht, st); - netsnmp_session_free(&session); + snmp_session_free(&session); /* Warning message sent already, just bail out */ RETURN_FALSE; } @@ -1335,7 +1354,7 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) php_free_objid_query(&objid_query, oid_ht, value_ht, st); if (session_less_mode) { - netsnmp_session_free(&session); + snmp_session_free(&session); } else { netsnmp_ds_set_boolean(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_PRINT_NUMERIC_ENUM, glob_snmp_object.enum_print); netsnmp_ds_set_boolean(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_QUICK_PRINT, glob_snmp_object.quick_print); @@ -1590,10 +1609,10 @@ PHP_METHOD(SNMP, __construct) /* handle re-open of snmp session */ if (snmp_object->session) { - netsnmp_session_free(&(snmp_object->session)); + snmp_session_free(&(snmp_object->session)); } - if (!netsnmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries)) { + if (!snmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries)) { return; } snmp_object->max_oids = 0; @@ -1618,7 +1637,7 @@ PHP_METHOD(SNMP, close) RETURN_THROWS(); } - netsnmp_session_free(&(snmp_object->session)); + snmp_session_free(&(snmp_object->session)); RETURN_TRUE; } @@ -1669,7 +1688,7 @@ PHP_METHOD(SNMP, setSecurity) RETURN_THROWS(); } - if (!netsnmp_session_set_security(snmp_object->session, a1, a2, a3, a4, a5, a6, a7)) { + if (!snmp_session_set_security(snmp_object->session, a1, a2, a3, a4, a5, a6, a7)) { /* Warning message sent already, just bail out */ RETURN_FALSE; } From 29d56a5b4fb694e76db53c06e8131cd6419cafeb Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 5 Jan 2025 16:14:37 +0000 Subject: [PATCH 02/10] add test --- ext/snmp/tests/snmp_session_error.phpt | 28 ++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 ext/snmp/tests/snmp_session_error.phpt diff --git a/ext/snmp/tests/snmp_session_error.phpt b/ext/snmp/tests/snmp_session_error.phpt new file mode 100644 index 0000000000000..91f1317401308 --- /dev/null +++ b/ext/snmp/tests/snmp_session_error.phpt @@ -0,0 +1,28 @@ +--TEST-- +SNMP::__construct checks +--CREDITS-- +Boris Lytochkin +--EXTENSIONS-- +snmp +--SKIPIF-- + +--FILE-- + +--EXPECTF-- + +Warning: SNMP::__construct(): Remote port must be between 0 and 65535 in %s on line %d + +Warning: SNMP::__construct(): Remote port must be between 0 and 65535 in %s on line %d + +Warning: SNMP::__construct(): hostname length must be lower than 128 in %s on line %d +OK From dc963f3f572b0ed7912433a9b823b82af329724e Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 5 Jan 2025 16:50:27 +0000 Subject: [PATCH 03/10] to exception --- ext/snmp/snmp.c | 6 +++--- ext/snmp/tests/snmp_session_error.phpt | 29 +++++++++++++++++--------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/ext/snmp/snmp.c b/ext/snmp/snmp.c index 1fbb60b91b79d..cc608d372852b 100644 --- a/ext/snmp/snmp.c +++ b/ext/snmp/snmp.c @@ -842,7 +842,7 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st struct sockaddr **res; if (ZSTR_LEN(hostname) >= MAX_NAME_LEN) { - php_error_docref(NULL, E_WARNING, "hostname length must be lower than %d", MAX_NAME_LEN); + zend_value_error("hostname length must be lower than %d", MAX_NAME_LEN); return false; } @@ -872,7 +872,7 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st char *pport = pptr + 2; tmp_port = atoi(pport); if (tmp_port < 0 || tmp_port > USHRT_MAX) { - php_error_docref(NULL, E_WARNING, "Remote port must be between 0 and %u", USHRT_MAX); + zend_value_error("remote port must be between 0 and %u", USHRT_MAX); return false; } remote_port = (unsigned short)tmp_port; @@ -887,7 +887,7 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st char *pport = pptr + 1; tmp_port = atoi(pport); if (tmp_port < 0 || tmp_port > USHRT_MAX) { - php_error_docref(NULL, E_WARNING, "Remote port must be between 0 and %u", USHRT_MAX); + zend_value_error("remote port must be between 0 and %u", USHRT_MAX); return false; } remote_port = (unsigned short)tmp_port; diff --git a/ext/snmp/tests/snmp_session_error.phpt b/ext/snmp/tests/snmp_session_error.phpt index 91f1317401308..61049572f2399 100644 --- a/ext/snmp/tests/snmp_session_error.phpt +++ b/ext/snmp/tests/snmp_session_error.phpt @@ -13,16 +13,25 @@ require_once(__DIR__.'/skipif.inc'); require_once(__DIR__.'/snmp_include.inc'); $longhostname = str_repeat($hostname4, 1_000_000); -new SNMP(SNMP::VERSION_1, "$hostname4:-1", $community, $timeout, $retries); -new SNMP(SNMP::VERSION_1, "$hostname4:65536", $community, $timeout, $retries); -new SNMP(SNMP::VERSION_1, "$longhostname:$port", $community, $timeout, $retries); +try { + new SNMP(SNMP::VERSION_1, "$hostname4:-1", $community, $timeout, $retries); +} catch (\ValueError $e) { + echo $e->getMessage(), PHP_EOL; +} +try { + new SNMP(SNMP::VERSION_1, "$hostname4:65536", $community, $timeout, $retries); +} catch (\ValueError $e) { + echo $e->getMessage(), PHP_EOL; +} +try { + new SNMP(SNMP::VERSION_1, "$longhostname:$port", $community, $timeout, $retries); +} catch (\ValueError $e) { + echo $e->getMessage(), PHP_EOL; +} echo "OK"; ?> ---EXPECTF-- - -Warning: SNMP::__construct(): Remote port must be between 0 and 65535 in %s on line %d - -Warning: SNMP::__construct(): Remote port must be between 0 and 65535 in %s on line %d - -Warning: SNMP::__construct(): hostname length must be lower than 128 in %s on line %d +--EXPECT-- +remote port must be between 0 and 65535 +remote port must be between 0 and 65535 +hostname length must be lower than 128 OK From 8dfc82dbc89f1ca678b9110427779d0b9d6c34b9 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sat, 25 Jan 2025 21:04:59 +0000 Subject: [PATCH 04/10] further initialisation type check and segfault fix. --- ext/snmp/snmp.c | 20 ++++++++++++++++---- ext/snmp/tests/snmp_session_error.phpt | 14 +++++++++++++- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/ext/snmp/snmp.c b/ext/snmp/snmp.c index cc608d372852b..4cf9bb88d96e3 100644 --- a/ext/snmp/snmp.c +++ b/ext/snmp/snmp.c @@ -832,7 +832,7 @@ static bool php_snmp_parse_oid( /* {{{ snmp_session_init allocates memory for session and session->peername, caller should free it manually using session_free() and efree() */ -static bool snmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, int timeout, int retries) +static bool snmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, zend_long timeout, zend_long retries) { php_snmp_session *session; char *pptr, *host_ptr; @@ -841,11 +841,23 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st struct sockaddr **psal; struct sockaddr **res; + *session_p = 0; + if (ZSTR_LEN(hostname) >= MAX_NAME_LEN) { zend_value_error("hostname length must be lower than %d", MAX_NAME_LEN); return false; } + if (timeout < -1 || timeout > LONG_MAX) { + zend_value_error("timeout must be between -1 and %ld", LONG_MAX); + return false; + } + + if (retries < -1 || retries > INT_MAX) { + zend_value_error("retries must be between -1 and %d", INT_MAX); + return false; + } + // TODO: Do not strip and re-add the port in peername? unsigned short remote_port = SNMP_PORT; int tmp_port; @@ -856,7 +868,7 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st snmp_sess_init(session); - session->version = version; + session->version = (long)version; session->peername = emalloc(MAX_NAME_LEN); /* we copy original hostname for further processing */ @@ -954,8 +966,8 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st session->community_len = ZSTR_LEN(community); } - session->retries = retries; - session->timeout = timeout; + session->retries = (int)retries; + session->timeout = (long)timeout; return true; } /* }}} */ diff --git a/ext/snmp/tests/snmp_session_error.phpt b/ext/snmp/tests/snmp_session_error.phpt index 61049572f2399..bb3e61594eef9 100644 --- a/ext/snmp/tests/snmp_session_error.phpt +++ b/ext/snmp/tests/snmp_session_error.phpt @@ -28,10 +28,22 @@ try { } catch (\ValueError $e) { echo $e->getMessage(), PHP_EOL; } +try { + new SNMP(SNMP::VERSION_1, "$hostname:$port", $community, PHP_INT_MIN, $retries); +} catch (\ValueError $e) { + echo $e->getMessage(), PHP_EOL; +} +try { + new SNMP(SNMP::VERSION_1, "$hostname:$port", $community, $timeout, PHP_INT_MAX); +} catch (\ValueError $e) { + echo $e->getMessage(), PHP_EOL; +} echo "OK"; ?> ---EXPECT-- +--EXPECTF-- remote port must be between 0 and 65535 remote port must be between 0 and 65535 hostname length must be lower than 128 +timeout must be between -1 and %d +retries must be between -1 and %d OK From 305631dceae669d3610dfb16d01e0299d82c8298 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 26 Jan 2025 06:06:06 +0000 Subject: [PATCH 05/10] revert renaming --- ext/snmp/snmp.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/ext/snmp/snmp.c b/ext/snmp/snmp.c index 4cf9bb88d96e3..fc64f9d3d64f0 100644 --- a/ext/snmp/snmp.c +++ b/ext/snmp/snmp.c @@ -153,7 +153,7 @@ static PHP_GINIT_FUNCTION(snmp) } \ } -static void snmp_session_free(php_snmp_session **session) /* {{{ */ +static void netsnmp_session_free(php_snmp_session **session) /* {{{ */ { if (*session) { PHP_SNMP_SESSION_FREE(peername); @@ -174,7 +174,7 @@ static void php_snmp_object_free_storage(zend_object *object) /* {{{ */ return; } - snmp_session_free(&(intern->session)); + netsnmp_session_free(&(intern->session)); zend_object_std_dtor(&intern->zo); } @@ -829,10 +829,10 @@ static bool php_snmp_parse_oid( } /* }}} */ -/* {{{ snmp_session_init +/* {{{ netsnmp_session_init allocates memory for session and session->peername, caller should free it manually using session_free() and efree() */ -static bool snmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, zend_long timeout, zend_long retries) +static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, zend_long timeout, zend_long retries) { php_snmp_session *session; char *pptr, *host_ptr; @@ -1320,14 +1320,14 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) } if (session_less_mode) { - if (!snmp_session_init(&session, version, a1, a2, timeout, retries)) { + if (!netsnmp_session_init(&session, version, a1, a2, timeout, retries)) { php_free_objid_query(&objid_query, oid_ht, value_ht, st); - snmp_session_free(&session); + netsnmp_session_free(&session); RETURN_FALSE; } if (version == SNMP_VERSION_3 && !snmp_session_set_security(session, a3, a4, a5, a6, a7, NULL, NULL)) { php_free_objid_query(&objid_query, oid_ht, value_ht, st); - snmp_session_free(&session); + netsnmp_session_free(&session); /* Warning message sent already, just bail out */ RETURN_FALSE; } @@ -1366,7 +1366,7 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) php_free_objid_query(&objid_query, oid_ht, value_ht, st); if (session_less_mode) { - snmp_session_free(&session); + netsnmp_session_free(&session); } else { netsnmp_ds_set_boolean(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_PRINT_NUMERIC_ENUM, glob_snmp_object.enum_print); netsnmp_ds_set_boolean(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_QUICK_PRINT, glob_snmp_object.quick_print); @@ -1621,10 +1621,10 @@ PHP_METHOD(SNMP, __construct) /* handle re-open of snmp session */ if (snmp_object->session) { - snmp_session_free(&(snmp_object->session)); + netsnmp_session_free(&(snmp_object->session)); } - if (!snmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries)) { + if (!netsnmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries)) { return; } snmp_object->max_oids = 0; @@ -1649,7 +1649,7 @@ PHP_METHOD(SNMP, close) RETURN_THROWS(); } - snmp_session_free(&(snmp_object->session)); + netsnmp_session_free(&(snmp_object->session)); RETURN_TRUE; } From 4d076cf467e968c3267da452391bec7032cc2b25 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 26 Jan 2025 06:52:36 +0000 Subject: [PATCH 06/10] changes from review --- ext/snmp/snmp.c | 45 +++++++++++++++++++++----- ext/snmp/tests/snmp_session_error.phpt | 24 ++++++++++++-- 2 files changed, 58 insertions(+), 11 deletions(-) diff --git a/ext/snmp/snmp.c b/ext/snmp/snmp.c index fc64f9d3d64f0..e788f718ef6a3 100644 --- a/ext/snmp/snmp.c +++ b/ext/snmp/snmp.c @@ -832,7 +832,7 @@ static bool php_snmp_parse_oid( /* {{{ netsnmp_session_init allocates memory for session and session->peername, caller should free it manually using session_free() and efree() */ -static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, zend_long timeout, zend_long retries) +static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, zend_long timeout, zend_long retries, int timeout_argument_offset) { php_snmp_session *session; char *pptr, *host_ptr; @@ -843,21 +843,41 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend *session_p = 0; + ZEND_ASSERT(hostname != NULL); + ZEND_ASSERT(community != NULL); + + if (zend_str_has_nul_byte(hostname)) { + zend_argument_value_error(2, "must not contain any null bytes"); + return false; + } + if (ZSTR_LEN(hostname) >= MAX_NAME_LEN) { - zend_value_error("hostname length must be lower than %d", MAX_NAME_LEN); + zend_argument_value_error(2, "length must be lower than %d", MAX_NAME_LEN); return false; } - if (timeout < -1 || timeout > LONG_MAX) { - zend_value_error("timeout must be between -1 and %ld", LONG_MAX); + if (zend_str_has_nul_byte(community)) { + zend_argument_value_error(3, "must not contain any null bytes"); return false; } - if (retries < -1 || retries > INT_MAX) { - zend_value_error("retries must be between -1 and %d", INT_MAX); + if (ZSTR_LEN(community) == 0) { + zend_argument_value_error(3, "cannot be empty"); return false; } + if (timeout_argument_offset != -1) { + if (timeout < -1 || timeout > LONG_MAX) { + zend_argument_value_error(timeout_argument_offset, "must be between -1 and %ld", LONG_MAX); + return false; + } + + if (retries < -1 || retries > INT_MAX) { + zend_argument_value_error(timeout_argument_offset, "must be between -1 and %d", INT_MAX); + return false; + } + } + // TODO: Do not strip and re-add the port in peername? unsigned short remote_port = SNMP_PORT; int tmp_port; @@ -1207,6 +1227,7 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) struct objid_query objid_query; php_snmp_session *session; int session_less_mode = (getThis() == NULL); + int timeout_argument_offset = -1; php_snmp_object *snmp_object; php_snmp_object glob_snmp_object; @@ -1233,6 +1254,8 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) Z_PARAM_LONG(timeout) Z_PARAM_LONG(retries) ZEND_PARSE_PARAMETERS_END(); + + timeout_argument_offset = 10; } else { /* SNMP_CMD_GET * SNMP_CMD_GETNEXT @@ -1251,6 +1274,8 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) Z_PARAM_LONG(timeout) Z_PARAM_LONG(retries) ZEND_PARSE_PARAMETERS_END(); + + timeout_argument_offset = 9; } } else { if (st & SNMP_CMD_SET) { @@ -1264,6 +1289,8 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) Z_PARAM_LONG(timeout) Z_PARAM_LONG(retries) ZEND_PARSE_PARAMETERS_END(); + + timeout_argument_offset = 6; } else { /* SNMP_CMD_GET * SNMP_CMD_GETNEXT @@ -1277,6 +1304,8 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) Z_PARAM_LONG(timeout) Z_PARAM_LONG(retries) ZEND_PARSE_PARAMETERS_END(); + + timeout_argument_offset = 4; } } } else { @@ -1320,7 +1349,7 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) } if (session_less_mode) { - if (!netsnmp_session_init(&session, version, a1, a2, timeout, retries)) { + if (!netsnmp_session_init(&session, version, a1, a2, timeout, retries, timeout_argument_offset)) { php_free_objid_query(&objid_query, oid_ht, value_ht, st); netsnmp_session_free(&session); RETURN_FALSE; @@ -1624,7 +1653,7 @@ PHP_METHOD(SNMP, __construct) netsnmp_session_free(&(snmp_object->session)); } - if (!netsnmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries)) { + if (!netsnmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries, 4)) { return; } snmp_object->max_oids = 0; diff --git a/ext/snmp/tests/snmp_session_error.phpt b/ext/snmp/tests/snmp_session_error.phpt index bb3e61594eef9..ae1fc9c24d287 100644 --- a/ext/snmp/tests/snmp_session_error.phpt +++ b/ext/snmp/tests/snmp_session_error.phpt @@ -38,12 +38,30 @@ try { } catch (\ValueError $e) { echo $e->getMessage(), PHP_EOL; } +try { + new SNMP(SNMP::VERSION_1, "\0$hostname:$port", $community, $timeout, $retries); +} catch (\ValueError $e) { + echo $e->getMessage(), PHP_EOL; +} +try { + new SNMP(SNMP::VERSION_1, "$hostname:$port", "", $timeout, $retries); +} catch (\ValueError $e) { + echo $e->getMessage(), PHP_EOL; +} +try { + new SNMP(SNMP::VERSION_1, "$hostname:$port", "\0$community", $timeout, $retries); +} catch (\ValueError $e) { + echo $e->getMessage(), PHP_EOL; +} echo "OK"; ?> --EXPECTF-- remote port must be between 0 and 65535 remote port must be between 0 and 65535 -hostname length must be lower than 128 -timeout must be between -1 and %d -retries must be between -1 and %d +SNMP::__construct(): Argument #2 ($hostname) length must be lower than 128 +SNMP::__construct(): Argument #4 ($timeout) must be between -1 and %d +SNMP::__construct(): Argument #4 ($timeout) must be between -1 and %d +SNMP::__construct(): Argument #2 ($hostname) must not contain any null bytes +SNMP::__construct(): Argument #3 ($community) cannot be empty +SNMP::__construct(): Argument #3 ($community) must not contain any null bytes OK From 051a82ab461340d730715ca13a26d244171a3431 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 26 Jan 2025 06:54:12 +0000 Subject: [PATCH 07/10] renaming --- ext/snmp/snmp.c | 24 ++++++++++++------------ ext/snmp/tests/snmp_session_error.phpt | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/ext/snmp/snmp.c b/ext/snmp/snmp.c index e788f718ef6a3..6b632fb7ce57e 100644 --- a/ext/snmp/snmp.c +++ b/ext/snmp/snmp.c @@ -153,7 +153,7 @@ static PHP_GINIT_FUNCTION(snmp) } \ } -static void netsnmp_session_free(php_snmp_session **session) /* {{{ */ +static void snmp_session_free(php_snmp_session **session) /* {{{ */ { if (*session) { PHP_SNMP_SESSION_FREE(peername); @@ -174,7 +174,7 @@ static void php_snmp_object_free_storage(zend_object *object) /* {{{ */ return; } - netsnmp_session_free(&(intern->session)); + snmp_session_free(&(intern->session)); zend_object_std_dtor(&intern->zo); } @@ -829,10 +829,10 @@ static bool php_snmp_parse_oid( } /* }}} */ -/* {{{ netsnmp_session_init +/* {{{ snmp_session_init allocates memory for session and session->peername, caller should free it manually using session_free() and efree() */ -static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, zend_long timeout, zend_long retries, int timeout_argument_offset) +static bool snmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, zend_long timeout, zend_long retries, int timeout_argument_offset) { php_snmp_session *session; char *pptr, *host_ptr; @@ -873,7 +873,7 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend } if (retries < -1 || retries > INT_MAX) { - zend_argument_value_error(timeout_argument_offset, "must be between -1 and %d", INT_MAX); + zend_argument_value_error(timeout_argument_offset + 1, "must be between -1 and %d", INT_MAX); return false; } } @@ -1349,14 +1349,14 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) } if (session_less_mode) { - if (!netsnmp_session_init(&session, version, a1, a2, timeout, retries, timeout_argument_offset)) { + if (!snmp_session_init(&session, version, a1, a2, timeout, retries, timeout_argument_offset)) { php_free_objid_query(&objid_query, oid_ht, value_ht, st); - netsnmp_session_free(&session); + snmp_session_free(&session); RETURN_FALSE; } if (version == SNMP_VERSION_3 && !snmp_session_set_security(session, a3, a4, a5, a6, a7, NULL, NULL)) { php_free_objid_query(&objid_query, oid_ht, value_ht, st); - netsnmp_session_free(&session); + snmp_session_free(&session); /* Warning message sent already, just bail out */ RETURN_FALSE; } @@ -1395,7 +1395,7 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version) php_free_objid_query(&objid_query, oid_ht, value_ht, st); if (session_less_mode) { - netsnmp_session_free(&session); + snmp_session_free(&session); } else { netsnmp_ds_set_boolean(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_PRINT_NUMERIC_ENUM, glob_snmp_object.enum_print); netsnmp_ds_set_boolean(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_QUICK_PRINT, glob_snmp_object.quick_print); @@ -1650,10 +1650,10 @@ PHP_METHOD(SNMP, __construct) /* handle re-open of snmp session */ if (snmp_object->session) { - netsnmp_session_free(&(snmp_object->session)); + snmp_session_free(&(snmp_object->session)); } - if (!netsnmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries, 4)) { + if (!snmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries, 4)) { return; } snmp_object->max_oids = 0; @@ -1678,7 +1678,7 @@ PHP_METHOD(SNMP, close) RETURN_THROWS(); } - netsnmp_session_free(&(snmp_object->session)); + snmp_session_free(&(snmp_object->session)); RETURN_TRUE; } diff --git a/ext/snmp/tests/snmp_session_error.phpt b/ext/snmp/tests/snmp_session_error.phpt index ae1fc9c24d287..69f8be506b8e3 100644 --- a/ext/snmp/tests/snmp_session_error.phpt +++ b/ext/snmp/tests/snmp_session_error.phpt @@ -60,7 +60,7 @@ remote port must be between 0 and 65535 remote port must be between 0 and 65535 SNMP::__construct(): Argument #2 ($hostname) length must be lower than 128 SNMP::__construct(): Argument #4 ($timeout) must be between -1 and %d -SNMP::__construct(): Argument #4 ($timeout) must be between -1 and %d +SNMP::__construct(): Argument #5 ($retries) must be between -1 and %d SNMP::__construct(): Argument #2 ($hostname) must not contain any null bytes SNMP::__construct(): Argument #3 ($community) cannot be empty SNMP::__construct(): Argument #3 ($community) must not contain any null bytes From b466f122fff54ddea7d84dd3cfc04c537c93be3a Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 26 Jan 2025 12:11:23 +0000 Subject: [PATCH 08/10] update comment --- ext/snmp/snmp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/snmp/snmp.c b/ext/snmp/snmp.c index 6b632fb7ce57e..14dd6c8f54e09 100644 --- a/ext/snmp/snmp.c +++ b/ext/snmp/snmp.c @@ -830,7 +830,7 @@ static bool php_snmp_parse_oid( /* }}} */ /* {{{ snmp_session_init - allocates memory for session and session->peername, caller should free it manually using session_free() and efree() + allocates memory for session and session->peername, caller should free it manually using snmp_session_free() and efree() */ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, zend_long timeout, zend_long retries, int timeout_argument_offset) { From 0fead784a59eff6c548943cab886a00006bbf74d Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 26 Jan 2025 16:57:16 +0000 Subject: [PATCH 09/10] last remaining changes. --- ext/snmp/snmp.c | 6 +++--- ext/snmp/tests/snmp_session_error.phpt | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ext/snmp/snmp.c b/ext/snmp/snmp.c index 14dd6c8f54e09..abee491720851 100644 --- a/ext/snmp/snmp.c +++ b/ext/snmp/snmp.c @@ -862,7 +862,7 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st } if (ZSTR_LEN(community) == 0) { - zend_argument_value_error(3, "cannot be empty"); + zend_argument_must_not_be_empty_error(3); return false; } @@ -904,7 +904,7 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st char *pport = pptr + 2; tmp_port = atoi(pport); if (tmp_port < 0 || tmp_port > USHRT_MAX) { - zend_value_error("remote port must be between 0 and %u", USHRT_MAX); + zend_argument_value_error(2, "remote port must be between 0 and %u", USHRT_MAX); return false; } remote_port = (unsigned short)tmp_port; @@ -919,7 +919,7 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st char *pport = pptr + 1; tmp_port = atoi(pport); if (tmp_port < 0 || tmp_port > USHRT_MAX) { - zend_value_error("remote port must be between 0 and %u", USHRT_MAX); + zend_argument_value_error(2, "remote port must be between 0 and %u", USHRT_MAX); return false; } remote_port = (unsigned short)tmp_port; diff --git a/ext/snmp/tests/snmp_session_error.phpt b/ext/snmp/tests/snmp_session_error.phpt index 69f8be506b8e3..6dc560bbd12a7 100644 --- a/ext/snmp/tests/snmp_session_error.phpt +++ b/ext/snmp/tests/snmp_session_error.phpt @@ -56,8 +56,8 @@ try { echo "OK"; ?> --EXPECTF-- -remote port must be between 0 and 65535 -remote port must be between 0 and 65535 +SNMP::__construct(): Argument #2 ($hostname) remote port must be between 0 and 65535 +SNMP::__construct(): Argument #2 ($hostname) remote port must be between 0 and 65535 SNMP::__construct(): Argument #2 ($hostname) length must be lower than 128 SNMP::__construct(): Argument #4 ($timeout) must be between -1 and %d SNMP::__construct(): Argument #5 ($retries) must be between -1 and %d From dc02a8fc4afcc1d2f939e0cc0f4841567be6046c Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 26 Jan 2025 17:01:13 +0000 Subject: [PATCH 10/10] fix test --- ext/snmp/tests/snmp_session_error.phpt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/snmp/tests/snmp_session_error.phpt b/ext/snmp/tests/snmp_session_error.phpt index 6dc560bbd12a7..f5e46ee4d936c 100644 --- a/ext/snmp/tests/snmp_session_error.phpt +++ b/ext/snmp/tests/snmp_session_error.phpt @@ -62,6 +62,6 @@ SNMP::__construct(): Argument #2 ($hostname) length must be lower than 128 SNMP::__construct(): Argument #4 ($timeout) must be between -1 and %d SNMP::__construct(): Argument #5 ($retries) must be between -1 and %d SNMP::__construct(): Argument #2 ($hostname) must not contain any null bytes -SNMP::__construct(): Argument #3 ($community) cannot be empty +SNMP::__construct(): Argument #3 ($community) must not be empty SNMP::__construct(): Argument #3 ($community) must not contain any null bytes OK