Fix GH-16959: snmpget modifies the `object_id` (as array). #16969

devnexen · 2024-11-27T19:18:15Z

Instead of modifying the zval, we use the zend_try_get_string.

ext/snmp/snmp.c

nielsdos · 2024-11-28T22:11:21Z

Feel free to ping me if this needs a re-review

devnexen · 2024-11-28T23:48:14Z

No need to get into too much details yet but is there room for improvements or does this approach look terrible ?

nielsdos

Looks like the right approach. Preliminary review without checking the details

nielsdos · 2024-11-29T08:06:19Z

ext/snmp/snmp.c

@@ -728,19 +728,22 @@ static bool php_snmp_parse_oid(
 						zval new;
 						ZVAL_COPY_VALUE(&new, tmp_type);
 						if (!try_convert_to_string(&new)) {
+							zend_string_release(tmp);


I would expect these to get freed by php_free_objid_query (ditto below)

nielsdos · 2024-11-29T08:06:44Z

ext/snmp/snmp.c

@@ -728,19 +728,22 @@ static bool php_snmp_parse_oid(
 						zval new;
 						ZVAL_COPY_VALUE(&new, tmp_type);
 						if (!try_convert_to_string(&new)) {


Why not zval_try_get_string? By using COPY_VALUE with a conversion you probably reach RC0 too soon.

Yes I was planning to go back to trying that again.

nielsdos · 2024-11-29T08:07:18Z

ext/snmp/tests/gh16959.phpt

+	echo $e->getMessage() . PHP_EOL;
+}
+try {
+	snmp2_set($hostname, $communityWrite, $bad_object_ids, array("toolongtype"), array(null));


Please also add a test with a non-interned string.
A trick to make one is using str_repeat("my string", random_int(1, 1)).

nielsdos · 2024-11-29T08:08:03Z

ext/snmp/snmp.c

 	if (session_less_mode) {
 		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);
 		netsnmp_ds_set_int(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_OID_OUTPUT_FORMAT, glob_snmp_object.oid_output_format);
 	}
+
+	php_free_objid_query(&objid_query, oid_ht, value_str, value_ht, st);


Any reason this was moved to the bottom of the function?

At one point with CI, it was complaining that glob_snmp_object would be used uninitialised.

nielsdos · 2024-11-30T19:56:17Z

ext/snmp/snmp.c

+static void php_snmp_zend_string_release_from_char_pointer(char *ptr) {
+	if (ptr) {
+		zend_string *pptr = (zend_string *)(ptr - XtOffsetOf(zend_string, val));
+		if (GC_REFCOUNT(pptr)) {


Why do you do this check?

nielsdos · 2024-11-30T19:56:43Z

ext/snmp/snmp.c

+		uint32_t i = 0, count = zend_hash_num_elements(oid_ht);
+
+		while (i < count) {


This is just a for loop with extra steps imho

nielsdos · 2024-11-30T19:57:07Z

ext/snmp/snmp.c

+			if (!arg->oid) {
+				break;
+			}
+			PHP_FREE_OBJID_VAL(arg);


The use of a macro is not necessary here

nielsdos · 2024-11-30T19:59:07Z

ext/snmp/snmp.c

+			if (value_ht && !value_str) { 						\
+				php_snmp_zend_string_release_from_char_pointer(arg->value); 	\
+			}									\
+			php_snmp_zend_string_release_from_char_pointer(&arg->type);		\


This is wrong, type is a char, this line should not be here

nielsdos · 2024-11-30T20:00:27Z

ext/snmp/snmp.c

 							zend_value_error("Type must be a single character");
-							efree(objid_query->vars);
+							zend_string_release(type);


Sadly GH does not allow me to make suggestions here, but you're leaking memory and making it too complicated.
Place this release above the if on line 748. Then, prior to releasing: read the first character and put it in a variable. Then remove pptr = ZSTR_VAL(type); and put the read character in line 755 by using the variable.

nielsdos · 2024-11-30T20:03:15Z

ext/snmp/snmp.c

+	}
+}
+
+static void php_free_objid_query(struct objid_query *objid_query, HashTable* oid_ht, zend_string *value_str, HashTable *value_ht, int st) {


I think it's a good idea to make value_ht and value_str const to indicate to the programmer that they are just used for checking.

nielsdos

one suggestion that you should verify, but otherwise lgtm

nielsdos · 2024-12-01T10:39:41Z

ext/snmp/snmp.c

+			if (!arg->oid) {
+				break;
+			}
+			if (value_ht && !value_str) {


I think you only need to check value_ht, and then you can remove the value_str argument.

I ll check a bit later, it s mainly because this but we ll see.

Yes I see, but note that the way the arguments are parsed only value_ht or value_str can be non-NULL:

php-src/ext/snmp/snmp.c

Line 1202 in 2c46d83

Z_PARAM_ARRAY_HT_OR_STR(value_ht, value_str)

Instead of modifying the zval, we use the zend_try_get_string.

Instead of modifying the zval, we use the zend_try_get_string. close phpGH-16969

github-actions bot added the Extension: snmp label Nov 27, 2024

devnexen linked an issue Nov 27, 2024 that may be closed by this pull request

Segmentation fault in snmpget #16959

Closed

devnexen marked this pull request as ready for review November 27, 2024 21:18

nielsdos requested changes Nov 27, 2024

View reviewed changes

ext/snmp/snmp.c Show resolved Hide resolved

devnexen marked this pull request as draft November 27, 2024 21:46

devnexen force-pushed the gh16959 branch 8 times, most recently from dc73754 to e2691d8 Compare November 28, 2024 12:26

nielsdos reviewed Nov 29, 2024

View reviewed changes

devnexen force-pushed the gh16959 branch 7 times, most recently from 30c826c to 215f9a9 Compare November 30, 2024 17:06

devnexen requested a review from nielsdos November 30, 2024 17:53

nielsdos requested changes Nov 30, 2024

View reviewed changes

devnexen force-pushed the gh16959 branch from 215f9a9 to 2c46d83 Compare November 30, 2024 20:21

devnexen marked this pull request as ready for review November 30, 2024 20:56

nielsdos approved these changes Dec 1, 2024

View reviewed changes

Fix phpGH-16959: snmpget modifies the object_id (as array).

31e0c87

Instead of modifying the zval, we use the zend_try_get_string.

devnexen force-pushed the gh16959 branch from 9d27d0a to 31e0c87 Compare December 1, 2024 13:13

nielsdos approved these changes Dec 1, 2024

View reviewed changes

devnexen closed this in 73ebc92 Dec 1, 2024

charmitro pushed a commit to wasix-org/php that referenced this pull request Mar 13, 2025

Fix phpGH-16959: snmpget modifies the object_id (as array).

6d74d34

Instead of modifying the zval, we use the zend_try_get_string. close phpGH-16969

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GH-16959: snmpget modifies the `object_id` (as array). #16969

Fix GH-16959: snmpget modifies the `object_id` (as array). #16969

devnexen commented Nov 27, 2024

nielsdos commented Nov 28, 2024

devnexen commented Nov 28, 2024

nielsdos left a comment

nielsdos Nov 29, 2024

nielsdos Nov 29, 2024

devnexen Nov 29, 2024

nielsdos Nov 29, 2024

nielsdos Nov 29, 2024

devnexen Nov 29, 2024

nielsdos Nov 30, 2024

nielsdos Nov 30, 2024

nielsdos Nov 30, 2024

nielsdos Nov 30, 2024

nielsdos Nov 30, 2024 •

edited

Loading

nielsdos Nov 30, 2024

nielsdos left a comment

nielsdos Dec 1, 2024

devnexen Dec 1, 2024

nielsdos Dec 1, 2024

		uint32_t i = 0, count = zend_hash_num_elements(oid_ht);

		while (i < count) {

Fix GH-16959: snmpget modifies the object_id (as array). #16969

Fix GH-16959: snmpget modifies the object_id (as array). #16969

Conversation

devnexen commented Nov 27, 2024

nielsdos commented Nov 28, 2024

devnexen commented Nov 28, 2024

nielsdos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix GH-16959: snmpget modifies the `object_id` (as array). #16969

Fix GH-16959: snmpget modifies the `object_id` (as array). #16969

nielsdos Nov 30, 2024 •

edited

Loading