Skip to contentSkip to conversation

resultgen: avoid null pointer dereference from realloc

PATCH In review Jeremy Cline 3 participants 4 messages Dec 14, 2020 – Dec 15, 2020

lore
Share & tools
lore

Jeremy Cline

author
#0 lore raw

PATCH 1/1 resultgen: avoid null pointer dereference from realloc

realloc() and friends return NULL if they fail; simplify the new_escaped_json_string() by allocating all the necessary memory up-front and checking for a failed allocation.

new_escaped_json_string() can already return NULL since json_oject_new_string_len() returns NULL for various undocumented error paths, and NULL is valid input for json_object_object_add(), which this new_escaped_json_string() is currently exclusively used with. Thus, returning NULL when memory allocation fails should be safe.

Signed-off-by Jeremy Cline <[email protected]>
---
 runner/resultgen.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
runner/resultgen.c +9 -2
index 3fe83b43..46007803 100644
@@ -412,14 +412,21 @@ static struct json_object *new_escaped_json_string(const char *buf, size_t len)
 	size_t strsize = 0;
 	size_t i;
 
+	/*
+	 * Test output may be garbage; strings passed to json-c need to be
+	 * UTF-8 encoded so any non-ASCII characters are converted to their
+	 * UTF-8 representation, which requires 2 bytes per character.
+	 */
+	str = malloc(len * 2);
+	if (!str)
+		return NULL;
+
 	for (i = 0; i < len; i++) {
 		if (buf[i] > 0 && buf[i] < 128) {
-			str = realloc(str, strsize + 1);
 			str[strsize] = buf[i];
 			++strsize;
 		} else {
 			/* Encode > 128 character to UTF-8. */
-			str = realloc(str, strsize + 2);
 			str[strsize] = ((unsigned char)buf[i] >> 6) | 0xC0;
 			str[strsize + 1] = ((unsigned char)buf[i] & 0x3F) | 0x80;
 			strsize += 2;
Signature · 6 lines
2.28.0

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev
CI
#1 lore raw

[igt-dev] ✗ Fi.CI.BAT: failure for resultgen: avoid null pointer dereference from realloc

== Series Details ==

== Series Details ==

Series: resultgen: avoid null pointer dereference from realloc
URL : https://patchwork.freedesktop.org/series/84920/
State : failure

== Summary ==

CI Bug Log - changes from IGT_5897 -> IGTPW_5290
====================================================

Summary
-------

**FAILURE**

Serious unknown changes coming with IGTPW_5290 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in IGTPW_5290, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/index.html

Possible new issues
-------------------

Here are the unknown changes that may have been introduced in IGTPW_5290:

### IGT changes ###

#### Possible regressions ####

* igt@gem_exec_suspend@basic-s3:
- fi-snb-2600: [PASS][1] -> [DMESG-WARN][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5897/fi-snb-2600/igt@[email protected]
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/fi-snb-2600/igt@[email protected]

Known issues
------------

Here are the changes found in IGTPW_5290 that come from known issues:

### IGT changes ###

#### Issues hit ####

* igt@gem_exec_suspend@basic-s3:
- fi-cfl-8109u: [PASS][3] -> [INCOMPLETE][4] ([i915#155] / [i915#2473])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5897/fi-cfl-8109u/igt@[email protected]
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/fi-cfl-8109u/igt@[email protected]

* igt@i915_pm_rpm@module-reload:
- fi-byt-j1900: [PASS][5] -> [INCOMPLETE][6] ([i915#142] / [i915#2405])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5897/fi-byt-j1900/igt@[email protected]
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/fi-byt-j1900/igt@[email protected]

* igt@i915_selftest@live@coherency:
- fi-gdg-551: [PASS][7] -> [DMESG-FAIL][8] ([i915#1748])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5897/fi-gdg-551/igt@i915_selftest@[email protected]
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/fi-gdg-551/igt@i915_selftest@[email protected]

* igt@prime_vgem@basic-read:
- fi-tgl-y: [PASS][9] -> [DMESG-WARN][10] ([i915#402]) +1 similar issue
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5897/fi-tgl-y/igt@[email protected]
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/fi-tgl-y/igt@[email protected]

* igt@runner@aborted:
- fi-cfl-8109u: NOTRUN -> [FAIL][11] ([i915#2426] / [i915#2722])
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/fi-cfl-8109u/igt@[email protected]
- fi-bdw-5557u: NOTRUN -> [FAIL][12] ([i915#2029] / [i915#2722])
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/fi-bdw-5557u/igt@[email protected]
- fi-snb-2600: NOTRUN -> [FAIL][13] ([i915#698])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/fi-snb-2600/igt@[email protected]
- fi-byt-j1900: NOTRUN -> [FAIL][14] ([i915#1814])
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/fi-byt-j1900/igt@[email protected]

#### Possible fixes ####

* igt@prime_vgem@basic-fence-flip:
- fi-tgl-y: [DMESG-WARN][15] ([i915#402]) -> [PASS][16]
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5897/fi-tgl-y/igt@[email protected]
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/fi-tgl-y/igt@[email protected]

#### Warnings ####

* igt@i915_pm_rpm@basic-rte:
- fi-kbl-guc: [FAIL][17] ([i915#579]) -> [SKIP][18] ([fdo#109271])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5897/fi-kbl-guc/igt@[email protected]
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/fi-kbl-guc/igt@[email protected]

[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[i915#142]: https://gitlab.freedesktop.org/drm/intel/issues/142
[i915#155]: https://gitlab.freedesktop.org/drm/intel/issues/155
[i915#1748]: https://gitlab.freedesktop.org/drm/intel/issues/1748
[i915#1814]: https://gitlab.freedesktop.org/drm/intel/issues/1814
[i915#2029]: https://gitlab.freedesktop.org/drm/intel/issues/2029
[i915#2405]: https://gitlab.freedesktop.org/drm/intel/issues/2405
[i915#2426]: https://gitlab.freedesktop.org/drm/intel/issues/2426
[i915#2473]: https://gitlab.freedesktop.org/drm/intel/issues/2473
[i915#2722]: https://gitlab.freedesktop.org/drm/intel/issues/2722
[i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
[i915#579]: https://gitlab.freedesktop.org/drm/intel/issues/579
[i915#698]: https://gitlab.freedesktop.org/drm/intel/issues/698

Participating hosts (42 -> 38)
------------------------------

Additional (1): fi-bdw-5557u
Missing (5): fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600 fi-dg1-1 fi-bdw-samus

Build changes
-------------

* CI: CI-20190529 -> None
* IGT: IGT_5897 -> IGTPW_5290

CI-20190529: 20190529
CI_DRM_9482: 279e4ca8a7117e617c498833deaa287b797e7d09 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_5290: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/index.html
IGT_5897: c88c4cc74620285079b7f0ae74a85076aeede760 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5290/index.html

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Reviewed-by on v1 1/1
#2 lore raw
In reply to Jeremy Cline
Quoted 11 lines · Jeremy Cline

realloc() and friends return NULL if they fail; simplify the new_escaped_json_string() by allocating all the necessary memory up-front and checking for a failed allocation.

new_escaped_json_string() can already return NULL since json_oject_new_string_len() returns NULL for various undocumented error paths, and NULL is valid input for json_object_object_add(), which this new_escaped_json_string() is currently exclusively used with. Thus, returning NULL when memory allocation fails should be safe.

Signed-off-by Jeremy Cline <[email protected]>

Nice cleanup, thanks.

Reviewed-by Petri Latvala <[email protected]>
Quoted 39 lines
---
 runner/resultgen.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
runner/resultgen.c +9 -2
index 3fe83b43..46007803 100644
@@ -412,14 +412,21 @@ static struct json_object *new_escaped_json_string(const char *buf, size_t len)
 	size_t strsize = 0;
 	size_t i;
 
+	/*
+	 * Test output may be garbage; strings passed to json-c need to be
+	 * UTF-8 encoded so any non-ASCII characters are converted to their
+	 * UTF-8 representation, which requires 2 bytes per character.
+	 */
+	str = malloc(len * 2);
+	if (!str)
+		return NULL;
+
 	for (i = 0; i < len; i++) {
 		if (buf[i] > 0 && buf[i] < 128) {
-			str = realloc(str, strsize + 1);
 			str[strsize] = buf[i];
 			++strsize;
 		} else {
 			/* Encode > 128 character to UTF-8. */
-			str = realloc(str, strsize + 2);
 			str[strsize] = ((unsigned char)buf[i] >> 6) | 0xC0;
 			str[strsize + 1] = ((unsigned char)buf[i] & 0x3F) | 0x80;
 			strsize += 2;
Signature · 6 lines
2.28.0

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

#3 lore raw
Quoted 11 lines · Petri Latvala
Quoted 10 lines · Jeremy Cline

realloc() and friends return NULL if they fail; simplify the new_escaped_json_string() by allocating all the necessary memory up-front and checking for a failed allocation.

new_escaped_json_string() can already return NULL since json_oject_new_string_len() returns NULL for various undocumented error paths, and NULL is valid input for json_object_object_add(), which this new_escaped_json_string() is currently exclusively used with. Thus, returning NULL when memory allocation fails should be safe.

Quoted 5 lines
Quoted 1 line
Signed-off-by Jeremy Cline <[email protected]>

Nice cleanup, thanks.

Reviewed-by Petri Latvala <[email protected]>

And now merged.

Signature · 5 lines
Petri Latvala
_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev