Skip to content

Commit cbe0144

Browse files
prateekbhujelprateekbhujel
authored
ext/snmp: promote invalid-input warnings to ValueError (#21319)
This now also specifies which argument is causing the error.
1 parent 88c658e commit cbe0144

File tree

7 files changed

+99
-81
lines changed

7 files changed

+99
-81
lines changed

‎ext/snmp/snmp.c‎

Lines changed: 50 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -655,7 +655,8 @@ static void php_free_objid_query(struct objid_query *objid_query, HashTable* oid
655655
*/
656656
static bool php_snmp_parse_oid(
657657
zval *object, int st, struct objid_query *objid_query, zend_string *oid_str, HashTable *oid_ht,
658-
zend_string *type_str, HashTable *type_ht, zend_string *value_str, HashTable *value_ht
658+
zend_string *type_str, HashTable *type_ht, zend_string *value_str, HashTable *value_ht,
659+
uint32_t oid_argument_offset, uint32_t type_argument_offset, uint32_t value_argument_offset
659660
) {
660661
char *pptr;
661662
uint32_t idx_type = 0, idx_value = 0;
@@ -682,7 +683,7 @@ static bool php_snmp_parse_oid(
682683
ZEND_ASSERT(type_str && value_str);
683684

684685
if (ZSTR_LEN(type_str) != 1) {
685-
zend_value_error("Type must be a single character");
686+
zend_argument_value_error(type_argument_offset, "must be a single character");
686687
efree(objid_query->vars);
687688
return false;
688689
}
@@ -693,7 +694,7 @@ static bool php_snmp_parse_oid(
693694
objid_query->count++;
694695
} else if (oid_ht) { /* we got objid array */
695696
if (zend_hash_num_elements(oid_ht) == 0) {
696-
zend_value_error("Array of object IDs must not be empty");
697+
zend_argument_value_error(oid_argument_offset, "must not be empty when passed as an array");
697698
return false;
698699
}
699700
objid_query->vars = (snmpobjarg *)safe_emalloc(sizeof(snmpobjarg), zend_hash_num_elements(oid_ht), 0);
@@ -738,14 +739,14 @@ static bool php_snmp_parse_oid(
738739
char ptype = *ZSTR_VAL(type);
739740
zend_string_release(type);
740741
if (len != 1) {
741-
zend_value_error("Type must be a single character");
742+
zend_argument_value_error(type_argument_offset, "must be a single character");
742743
php_free_objid_query(objid_query, oid_ht, value_ht, st);
743744
return false;
744745
}
745746
objid_query->vars[objid_query->count].type = ptype;
746747
idx_type++;
747748
} else {
748-
php_error_docref(NULL, E_WARNING, "'%s': no type set", ZSTR_VAL(tmp));
749+
zend_argument_value_error(type_argument_offset, "must contain a type for object ID '%s'", ZSTR_VAL(tmp));
749750
php_free_objid_query(objid_query, oid_ht, value_ht, st);
750751
return false;
751752
}
@@ -780,7 +781,7 @@ static bool php_snmp_parse_oid(
780781
objid_query->vars[objid_query->count].value = ZSTR_VAL(tmp);
781782
idx_value++;
782783
} else {
783-
php_error_docref(NULL, E_WARNING, "'%s': no value set", ZSTR_VAL(tmp));
784+
zend_argument_value_error(value_argument_offset, "must contain a value for object ID '%s'", ZSTR_VAL(tmp));
784785
php_free_objid_query(objid_query, oid_ht, value_ht, st);
785786
return false;
786787
}
@@ -827,7 +828,7 @@ static bool php_snmp_parse_oid(
827828
/* {{{ snmp_session_init
828829
allocates memory for session and session->peername, caller should free it manually using snmp_session_free() and efree()
829830
*/
830-
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)
831+
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 hostname_argument_offset, int timeout_argument_offset)
831832
{
832833
php_snmp_session *session;
833834
char *pptr, *host_ptr;
@@ -841,23 +842,13 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st
841842
ZEND_ASSERT(hostname != NULL);
842843
ZEND_ASSERT(community != NULL);
843844

844-
if (zend_str_has_nul_byte(hostname)) {
845-
zend_argument_value_error(2, "must not contain any null bytes");
846-
return false;
847-
}
848-
849845
if (ZSTR_LEN(hostname) >= MAX_NAME_LEN) {
850-
zend_argument_value_error(2, "length must be lower than %d", MAX_NAME_LEN);
851-
return false;
852-
}
853-
854-
if (zend_str_has_nul_byte(community)) {
855-
zend_argument_value_error(3, "must not contain any null bytes");
846+
zend_argument_value_error(hostname_argument_offset, "length must be lower than %d", MAX_NAME_LEN);
856847
return false;
857848
}
858849

859850
if (ZSTR_LEN(community) == 0) {
860-
zend_argument_must_not_be_empty_error(3);
851+
zend_argument_must_not_be_empty_error(hostname_argument_offset + 1);
861852
return false;
862853
}
863854

@@ -899,22 +890,22 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st
899890
char *pport = pptr + 2;
900891
tmp_port = atoi(pport);
901892
if (tmp_port < 0 || tmp_port > USHRT_MAX) {
902-
zend_argument_value_error(2, "remote port must be between 0 and %u", USHRT_MAX);
893+
zend_argument_value_error(hostname_argument_offset, "remote port must be between 0 and %u", USHRT_MAX);
903894
return false;
904895
}
905896
remote_port = (unsigned short)tmp_port;
906897
}
907898
*pptr = '\0';
908899
} else {
909-
php_error_docref(NULL, E_WARNING, "Malformed IPv6 address, closing square bracket missing");
900+
zend_argument_value_error(hostname_argument_offset, "has a malformed IPv6 address, closing square bracket missing");
910901
return false;
911902
}
912903
} else { /* IPv4 address */
913904
if ((pptr = strchr(host_ptr, ':'))) {
914905
char *pport = pptr + 1;
915906
tmp_port = atoi(pport);
916907
if (tmp_port < 0 || tmp_port > USHRT_MAX) {
917-
zend_argument_value_error(2, "remote port must be between 0 and %u", USHRT_MAX);
908+
zend_argument_value_error(hostname_argument_offset, "remote port must be between 0 and %u", USHRT_MAX);
918909
return false;
919910
}
920911
remote_port = (unsigned short)tmp_port;
@@ -1123,14 +1114,13 @@ static ZEND_ATTRIBUTE_NONNULL bool snmp_session_gen_sec_key(struct snmp_session
11231114
/* }}} */
11241115

11251116
/* {{{ Set context Engine Id in the snmpv3 session */
1126-
static bool snmp_session_set_contextEngineID(struct snmp_session *s, zend_string * contextEngineID)
1117+
static bool snmp_session_set_contextEngineID(struct snmp_session *s, zend_string * contextEngineID, uint32_t contextEngineID_argument_offset)
11271118
{
11281119
size_t ebuf_len = 32, eout_len = 0;
11291120
uint8_t *ebuf = (uint8_t *) emalloc(ebuf_len);
11301121

11311122
if (!snmp_hex_to_binary(&ebuf, &ebuf_len, &eout_len, 1, ZSTR_VAL(contextEngineID))) {
1132-
// TODO Promote to Error?
1133-
php_error_docref(NULL, E_WARNING, "Bad engine ID value '%s'", ZSTR_VAL(contextEngineID));
1123+
zend_argument_value_error(contextEngineID_argument_offset, "must be a valid context engine ID");
11341124
efree(ebuf);
11351125
return false;
11361126
}
@@ -1145,11 +1135,14 @@ static bool snmp_session_set_contextEngineID(struct snmp_session *s, zend_string
11451135
}
11461136
/* }}} */
11471137

1148-
/* {{{ Set all snmpv3-related security options */
1138+
/* {{{ Set all snmpv3-related security options
1139+
* auth_protocol_argnum and contextEngineID_argument_offset are the userland
1140+
* argument numbers used for error reporting.
1141+
*/
11491142
static ZEND_ATTRIBUTE_NONNULL_ARGS(2) bool snmp_session_set_security(struct snmp_session *session, zend_string *sec_level,
11501143
zend_string *auth_protocol, zend_string *auth_passphrase, zend_string *priv_protocol,
11511144
zend_string *priv_passphrase, zend_string *contextName, zend_string *contextEngineID,
1152-
uint32_t auth_protocol_argnum)
1145+
uint32_t auth_protocol_argnum, uint32_t contextEngineID_argument_offset)
11531146
{
11541147

11551148
/* Setting the security level. */
@@ -1215,7 +1208,7 @@ static ZEND_ATTRIBUTE_NONNULL_ARGS(2) bool snmp_session_set_security(struct snmp
12151208
}
12161209

12171210
/* Setting contextEngineIS if specified */
1218-
if (contextEngineID && ZSTR_LEN(contextEngineID) && !snmp_session_set_contextEngineID(session, contextEngineID)) {
1211+
if (contextEngineID && ZSTR_LEN(contextEngineID) && !snmp_session_set_contextEngineID(session, contextEngineID, contextEngineID_argument_offset)) {
12191212
/* Warning message sent already, just bail out */
12201213
return false;
12211214
}
@@ -1243,6 +1236,7 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version)
12431236
php_snmp_session *session;
12441237
int session_less_mode = (getThis() == NULL);
12451238
int timeout_argument_offset = -1;
1239+
uint32_t oid_argument_offset = 1, type_argument_offset = 0, value_argument_offset = 0;
12461240
php_snmp_object *snmp_object;
12471241
php_snmp_object glob_snmp_object;
12481242

@@ -1255,8 +1249,8 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version)
12551249
if (version == SNMP_VERSION_3) {
12561250
if (st & SNMP_CMD_SET) {
12571251
ZEND_PARSE_PARAMETERS_START(10, 12)
1258-
Z_PARAM_STR(a1)
1259-
Z_PARAM_STR(a2)
1252+
Z_PARAM_PATH_STR(a1)
1253+
Z_PARAM_PATH_STR(a2)
12601254
Z_PARAM_STR(a3)
12611255
Z_PARAM_STR(a4)
12621256
Z_PARAM_STR(a5)
@@ -1271,14 +1265,17 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version)
12711265
ZEND_PARSE_PARAMETERS_END();
12721266

12731267
timeout_argument_offset = 10;
1268+
oid_argument_offset = 8;
1269+
type_argument_offset = 9;
1270+
value_argument_offset = 10;
12741271
} else {
12751272
/* SNMP_CMD_GET
12761273
* SNMP_CMD_GETNEXT
12771274
* SNMP_CMD_WALK
12781275
*/
12791276
ZEND_PARSE_PARAMETERS_START(8, 10)
1280-
Z_PARAM_STR(a1)
1281-
Z_PARAM_STR(a2)
1277+
Z_PARAM_PATH_STR(a1)
1278+
Z_PARAM_PATH_STR(a2)
12821279
Z_PARAM_STR(a3)
12831280
Z_PARAM_STR(a4)
12841281
Z_PARAM_STR(a5)
@@ -1291,12 +1288,13 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version)
12911288
ZEND_PARSE_PARAMETERS_END();
12921289

12931290
timeout_argument_offset = 9;
1291+
oid_argument_offset = 8;
12941292
}
12951293
} else {
12961294
if (st & SNMP_CMD_SET) {
12971295
ZEND_PARSE_PARAMETERS_START(5, 7)
1298-
Z_PARAM_STR(a1)
1299-
Z_PARAM_STR(a2)
1296+
Z_PARAM_PATH_STR(a1)
1297+
Z_PARAM_PATH_STR(a2)
13001298
Z_PARAM_ARRAY_HT_OR_STR(oid_ht, oid_str)
13011299
Z_PARAM_ARRAY_HT_OR_STR(type_ht, type_str)
13021300
Z_PARAM_ARRAY_HT_OR_STR(value_ht, value_str)
@@ -1306,21 +1304,25 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version)
13061304
ZEND_PARSE_PARAMETERS_END();
13071305

13081306
timeout_argument_offset = 6;
1307+
oid_argument_offset = 3;
1308+
type_argument_offset = 4;
1309+
value_argument_offset = 5;
13091310
} else {
13101311
/* SNMP_CMD_GET
13111312
* SNMP_CMD_GETNEXT
13121313
* SNMP_CMD_WALK
13131314
*/
13141315
ZEND_PARSE_PARAMETERS_START(3, 5)
1315-
Z_PARAM_STR(a1)
1316-
Z_PARAM_STR(a2)
1316+
Z_PARAM_PATH_STR(a1)
1317+
Z_PARAM_PATH_STR(a2)
13171318
Z_PARAM_ARRAY_HT_OR_STR(oid_ht, oid_str)
13181319
Z_PARAM_OPTIONAL
13191320
Z_PARAM_LONG(timeout)
13201321
Z_PARAM_LONG(retries)
13211322
ZEND_PARSE_PARAMETERS_END();
13221323

13231324
timeout_argument_offset = 4;
1325+
oid_argument_offset = 3;
13241326
}
13251327
}
13261328
} else {
@@ -1330,6 +1332,8 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version)
13301332
Z_PARAM_ARRAY_HT_OR_STR(type_ht, type_str)
13311333
Z_PARAM_ARRAY_HT_OR_STR(value_ht, value_str)
13321334
ZEND_PARSE_PARAMETERS_END();
1335+
type_argument_offset = 2;
1336+
value_argument_offset = 3;
13331337
} else if (st & SNMP_CMD_WALK) {
13341338
ZEND_PARSE_PARAMETERS_START(1, 4)
13351339
Z_PARAM_ARRAY_HT_OR_STR(oid_ht, oid_str)
@@ -1359,20 +1363,21 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version)
13591363
}
13601364
}
13611365

1362-
if (!php_snmp_parse_oid(getThis(), st, &objid_query, oid_str, oid_ht, type_str, type_ht, value_str, value_ht)) {
1366+
if (!php_snmp_parse_oid(getThis(), st, &objid_query, oid_str, oid_ht, type_str, type_ht, value_str, value_ht,
1367+
oid_argument_offset, type_argument_offset, value_argument_offset)) {
13631368
RETURN_FALSE;
13641369
}
13651370

13661371
if (session_less_mode) {
1367-
if (!snmp_session_init(&session, version, a1, a2, timeout, retries, timeout_argument_offset)) {
1372+
if (!snmp_session_init(&session, version, a1, a2, timeout, retries, 1, timeout_argument_offset)) {
13681373
php_free_objid_query(&objid_query, oid_ht, value_ht, st);
13691374
snmp_session_free(&session);
13701375
RETURN_FALSE;
13711376
}
1372-
if (version == SNMP_VERSION_3 && !snmp_session_set_security(session, a3, a4, a5, a6, a7, NULL, NULL, 4)) {
1377+
if (version == SNMP_VERSION_3 && !snmp_session_set_security(session, a3, a4, a5, a6, a7, NULL, NULL, 4, 0)) {
13731378
php_free_objid_query(&objid_query, oid_ht, value_ht, st);
13741379
snmp_session_free(&session);
1375-
/* Warning message sent already, just bail out */
1380+
/* An error has already been emitted, just bail out. */
13761381
RETURN_FALSE;
13771382
}
13781383
} else {
@@ -1645,7 +1650,7 @@ PHP_METHOD(SNMP, __construct)
16451650

16461651
snmp_object = Z_SNMP_P(object);
16471652

1648-
if (zend_parse_parameters(ZEND_NUM_ARGS(), "lSS|ll", &version, &a1, &a2, &timeout, &retries) == FAILURE) {
1653+
if (zend_parse_parameters(ZEND_NUM_ARGS(), "lPP|ll", &version, &a1, &a2, &timeout, &retries) == FAILURE) {
16491654
RETURN_THROWS();
16501655
}
16511656

@@ -1664,7 +1669,7 @@ PHP_METHOD(SNMP, __construct)
16641669
snmp_session_free(&(snmp_object->session));
16651670
}
16661671

1667-
if (!snmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries, 4)) {
1672+
if (!snmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries, 2, 4)) {
16681673
return;
16691674
}
16701675
snmp_object->max_oids = 0;
@@ -1738,8 +1743,9 @@ PHP_METHOD(SNMP, setSecurity)
17381743
RETURN_THROWS();
17391744
}
17401745

1741-
if (!snmp_session_set_security(snmp_object->session, a1, a2, a3, a4, a5, a6, a7, 2)) {
1742-
/* Warning message sent already, just bail out */
1746+
/* authProtocol is argument #2 and contextEngineId is argument #7. */
1747+
if (!snmp_session_set_security(snmp_object->session, a1, a2, a3, a4, a5, a6, a7, 2, 7)) {
1748+
/* An error has already been emitted, just bail out. */
17431749
RETURN_FALSE;
17441750
}
17451751
RETURN_TRUE;

‎ext/snmp/tests/gh16959.phpt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,5 +66,5 @@ array(4) {
6666
}
6767
Object of class stdClass could not be converted to string
6868
Object of class stdClass could not be converted to string
69-
Type must be a single character
70-
Type must be a single character
69+
snmp2_set(): Argument #4 ($type) must be a single character
70+
snmp2_set(): Argument #4 ($type) must be a single character

‎ext/snmp/tests/ipv6.phpt‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,12 @@ snmp_set_quick_print(false);
2222
snmp_set_valueretrieval(SNMP_VALUE_PLAIN);
2323

2424
var_dump(snmpget($hostname6_port, $community, '.1.3.6.1.2.1.1.1.0'));
25-
var_dump(snmpget('[dead:beef::', $community, '.1.3.6.1.2.1.1.1.0'));
25+
try {
26+
var_dump(snmpget('[dead:beef::', $community, '.1.3.6.1.2.1.1.1.0'));
27+
} catch (\ValueError $e) {
28+
echo $e->getMessage() . \PHP_EOL;
29+
}
2630
?>
2731
--EXPECTF--
2832
string(%d) "%s"
29-
30-
Warning: snmpget(): Malformed IPv6 address, closing square bracket missing in %s on line %d
31-
bool(false)
33+
snmpget(): Argument #1 ($hostname) has a malformed IPv6 address, closing square bracket missing

‎ext/snmp/tests/snmp-object-setSecurity_error.phpt‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,11 @@ try {
5353

5454
var_dump($session->setSecurity('authPriv', 'MD5', $auth_pass, 'AES', ''));
5555
var_dump($session->setSecurity('authPriv', 'MD5', $auth_pass, 'AES', 'ty'));
56-
var_dump($session->setSecurity('authPriv', 'MD5', $auth_pass, 'AES', 'test12345', 'context', 'dsa'));
56+
try {
57+
var_dump($session->setSecurity('authPriv', 'MD5', $auth_pass, 'AES', 'test12345', 'context', 'dsa'));
58+
} catch (\ValueError $e) {
59+
echo $e->getMessage() . \PHP_EOL;
60+
}
5761

5862
var_dump($session->close());
5963

@@ -76,7 +80,5 @@ bool(false)
7680

7781
Warning: SNMP::setSecurity(): Error generating a key for privacy pass phrase 'ty': Generic error (The supplied password length is too short.) in %s on line %d
7882
bool(false)
79-
80-
Warning: SNMP::setSecurity(): Bad engine ID value 'dsa' in %s on line %d
81-
bool(false)
83+
SNMP::setSecurity(): Argument #7 ($contextEngineId) must be a valid context engine ID
8284
bool(true)

‎ext/snmp/tests/snmp2_get.phpt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ var_dump(snmp2_get($hostname, $community, array('.1.3.6.1.2.1.1.1.0', '.1.3.6.1.
5454
--EXPECTF--
5555
Checking error handling
5656
Empty OID array
57-
Array of object IDs must not be empty
57+
snmp2_get(): Argument #3 ($object_id) must not be empty when passed as an array
5858
Checking working
5959
Single OID
6060
string(%d) "%s"

0 commit comments

Comments
 (0)