Skip to contentSkip to conversation

tests/testdisplay: fix heap overflow

PATCH In review Simon Ser 4 participants 7 messages Mar 20, 2019 – Mar 25, 2019

lore
Share & tools
lore

Simon Ser

author
#0 lore raw

PATCH v3 1/1 tests/testdisplay: fix heap overflow

v2: also simplify the code by using dirname(3).

v3: dirname may modify in-place its argument, duplicate the string

Signed-off-by Simon Ser <[email protected]>

---

We can't free() the return value of dirname(), because it might be statically allocated.

Also, we don't want to chdir(NULL).

 tests/testdisplay.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)
tests/testdisplay.c +8 -14
index b3657264..67d1b68a 100644
@@ -51,6 +51,7 @@
 #include <cairo.h>
 #include <errno.h>
 #include <getopt.h>
+#include <libgen.h>
 #include <math.h>
 #include <stdint.h>
 #include <stdbool.h>

@@ -563,24 +564,17 @@ static gboolean input_event(GIOChannel *source, GIOCondition condition,
 	return TRUE;
 }
 
-static void enter_exec_path( char **argv )
+static void enter_exec_path(const char **argv)
 {
-	char *exec_path = NULL;
-	char *pos = NULL;
-	short len_path = 0;
+	char *argv0, *exec_path;
 	int ret;
 
-	len_path = strlen( argv[0] );
-	exec_path = (char*) malloc(len_path);
-
-	memcpy(exec_path, argv[0], len_path);
-	pos = strrchr(exec_path, '/');
-	if (pos != NULL)
-		*(pos+1) = '\0';
-
+	argv0 = strdup(argv[0]);
+	igt_assert(argv0);
+	exec_path = dirname(argv0);
 	ret = chdir(exec_path);
 	igt_assert_eq(ret, 0);
-	free(exec_path);
+	free(argv0);
 }
 
 static void restore_termio_mode(int sig)

@@ -626,7 +620,7 @@ int main(int argc, char **argv)
 
 	igt_skip_on_simulation();
 
-	enter_exec_path( argv );
+	enter_exec_path((const char **) argv);
 
 	while ((c = getopt_long(argc, argv, optstr, long_opts, NULL)) != -1) {
 		switch (c) {
Signature · 17 lines
2.21.0

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

Quoting Simon Ser (2019-03-20 11:48:57)

tests/testdisplay.c static gboolean input_event(GIOChannel *source, GIOCondition condition, · line 576

Quoted 5 lines
tests/testdisplay.c +3 -0
+       argv0 = strdup(argv[0]);
+       igt_assert(argv0);
+       exec_path = dirname(argv0);
        ret = chdir(exec_path);
        igt_assert_eq(ret, 0);

One should ask Petri if igt_assert_eq() is even legal inside the helper (i.e. outside of igt_main and igt_subtest).

tests/testdisplay.c static gboolean input_event(GIOChannel *source, GIOCondition condition, · line 577

Quoted 2 lines
tests/testdisplay.c +1 -1
-       free(exec_path);
+       free(argv0);

Ok, looks like dirname(e) doesn't die with NULL.

tests/testdisplay.c int main(int argc, char **argv) · line 623

Quoted 9 lines

}

static void restore_termio_mode(int sig)

tests/testdisplay.c +1 -1
@@ -626,7 +620,7 @@ int main(int argc, char **argv)
 
        igt_skip_on_simulation();
 
-       enter_exec_path( argv );
+       enter_exec_path((const char **) argv);

Sigh, I forget about C being unhelpful.

Your call whether that cast is worth it.

Reviewed-by Chris Wilson <[email protected]>

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

#2 lore raw
Quoted 9 lines · Chris Wilson

Quoting Simon Ser (2019-03-20 11:48:57)

tests/testdisplay.c static gboolean input_event(GIOChannel *source, GIOCondition condition, · line 576

Quoted 5 lines
tests/testdisplay.c +3 -0
+       argv0 = strdup(argv[0]);
+       igt_assert(argv0);
+       exec_path = dirname(argv0);
        ret = chdir(exec_path);
        igt_assert_eq(ret, 0);

One should ask Petri if igt_assert_eq() is even legal inside the helper (i.e. outside of igt_main and igt_subtest).

*opens testdisplay.c*

*finds main()*

*runs away screaming*

Short answer: It's not legal there

Long answer:
It would be legal there if appropriate steps are taken to ensure IGT core knows it's a test without subtests. testdisplay, being a dinosaur that hasn't realized it's pushing up the daisies, doesn't use igt_simple_main, or call igt_simple_init_parse_opts, or otherwise do the common things any recently written test is doing.

It's also calling igt_skip_on_simulation in just about the only possible context where it's not legal.

Note to self: Hurry up with removing all custom main functions.

Signature · 5 lines
Petri Latvala
_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev
author
#3 lore raw
Quoted 29 lines · Petri Latvala
Quoted 10 lines · Chris Wilson

Quoting Simon Ser (2019-03-20 11:48:57)

tests/testdisplay.c static gboolean input_event(GIOChannel *source, GIOCondition condition, · line 576

Quoted 5 lines
tests/testdisplay.c +3 -0
+       argv0 = strdup(argv[0]);
+       igt_assert(argv0);
+       exec_path = dirname(argv0);
        ret = chdir(exec_path);
        igt_assert_eq(ret, 0);

One should ask Petri if igt_assert_eq() is even legal inside the helper
(i.e. outside of igt_main and igt_subtest).

*opens testdisplay.c*

*finds main()*

*runs away screaming*

Short answer: It's not legal there

Long answer:
It would be legal there if appropriate steps are taken to ensure IGT core knows it's a test without subtests. testdisplay, being a dinosaur
that hasn't realized it's pushing up the daisies, doesn't use igt_simple_main, or call igt_simple_init_parse_opts, or otherwise do the common things any recently written test is doing.

Quoted 4 lines

It's also calling igt_skip_on_simulation in just about the only possible context where it's not legal.

Note to self: Hurry up with removing all custom main functions.

Hmm. So would you prefer to add an error return value to this function, or to just continue to use these even if they don't work and fix everything in a later commit?
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

#4 lore raw
Quoted 30 lines · Simon
Quoted 29 lines · Petri Latvala
Quoted 10 lines · Chris Wilson

Quoting Simon Ser (2019-03-20 11:48:57)

tests/testdisplay.c static gboolean input_event(GIOChannel *source, GIOCondition condition, · line 576

Quoted 5 lines
tests/testdisplay.c +3 -0
+       argv0 = strdup(argv[0]);
+       igt_assert(argv0);
+       exec_path = dirname(argv0);
        ret = chdir(exec_path);
        igt_assert_eq(ret, 0);

One should ask Petri if igt_assert_eq() is even legal inside the helper
(i.e. outside of igt_main and igt_subtest).

*opens testdisplay.c*

*finds main()*

*runs away screaming*

Short answer: It's not legal there

Long answer:
It would be legal there if appropriate steps are taken to ensure IGT core knows it's a test without subtests. testdisplay, being a dinosaur
that hasn't realized it's pushing up the daisies, doesn't use igt_simple_main, or call igt_simple_init_parse_opts, or otherwise do the common things any recently written test is doing.

Quoted 8 lines
Quoted 4 lines

It's also calling igt_skip_on_simulation in just about the only possible context where it's not legal.

Note to self: Hurry up with removing all custom main functions.

Hmm. So would you prefer to add an error return value to this function, or to just continue to use these even if they don't work and fix everything in a later commit?

No no, the patch is good. All that is wrong with the code has already been wrong, and will be fixed by replacing its custom main() with igt_simple_main_custom_argv_handling (implementation pending).

Signature · 5 lines
Petri Latvala
_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev
#5 lore raw
Quoted 36 lines · Petri Latvala
Quoted 35 lines · Simon
Quoted 33 lines · Petri Latvala
Quoted 10 lines · Chris Wilson

Quoting Simon Ser (2019-03-20 11:48:57)

tests/testdisplay.c static gboolean input_event(GIOChannel *source, GIOCondition condition, · line 576

Quoted 5 lines
tests/testdisplay.c +3 -0
+       argv0 = strdup(argv[0]);
+       igt_assert(argv0);
+       exec_path = dirname(argv0);
        ret = chdir(exec_path);
        igt_assert_eq(ret, 0);

One should ask Petri if igt_assert_eq() is even legal inside the helper
(i.e. outside of igt_main and igt_subtest).

*opens testdisplay.c*

*finds main()*

*runs away screaming*

Short answer: It's not legal there

Long answer:
It would be legal there if appropriate steps are taken to ensure IGT core knows it's a test without subtests. testdisplay, being a dinosaur
that hasn't realized it's pushing up the daisies, doesn't use igt_simple_main, or call igt_simple_init_parse_opts, or otherwise do the common things any recently written test is doing.

It's also calling igt_skip_on_simulation in just about the only possible context where it's not legal.

Note to self: Hurry up with removing all custom main functions.

Quoted 8 lines
Quoted 3 lines

Hmm. So would you prefer to add an error return value to this function, or to just continue to use these even if they don't work and fix everything in a later commit?

No no, the patch is good. All that is wrong with the code has already been wrong, and will be fixed by replacing its custom main() with igt_simple_main_custom_argv_handling (implementation pending).

And merged.

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

[igt-dev] ✓ Fi.CI.BAT: success for tests/testdisplay: fix heap overflow (rev2)

In reply to Simon Ser
== Series Details ==

== Series Details ==

Series: tests/testdisplay: fix heap overflow (rev2)
URL : https://patchwork.freedesktop.org/series/58219/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5780 -> IGTPW_2667
====================================================

Summary
-------

**SUCCESS**

No regressions found.

External URL: https://patchwork.freedesktop.org/api/1.0/series/58219/revisions/2/mbox/

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

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

### IGT changes ###

#### Issues hit ####

* igt@gem_exec_basic@basic-bsd2:
- fi-kbl-7500u: NOTRUN -> SKIP [fdo#109271] +9

* igt@kms_chamelium@dp-crc-fast:
- fi-kbl-7500u: NOTRUN -> DMESG-WARN [fdo#103841]

* igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
- fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362]

* igt@kms_psr@primary_mmap_gtt:
- fi-blb-e6850: NOTRUN -> SKIP [fdo#109271] +27

* igt@prime_vgem@basic-fence-flip:
- fi-gdg-551: PASS -> DMESG-FAIL [fdo#103182]

* igt@runner@aborted:
- fi-kbl-7500u: NOTRUN -> FAIL [fdo#103841]
- fi-apl-guc: NOTRUN -> FAIL [fdo#108622] / [fdo#109720]

#### Possible fixes ####

* igt@i915_selftest@live_uncore:
- fi-ivb-3770: DMESG-FAIL [fdo#110210] -> PASS

* igt@kms_busy@basic-flip-a:
- fi-gdg-551: FAIL [fdo#103182] -> PASS

* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- fi-blb-e6850: INCOMPLETE [fdo#107718] -> PASS

[fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
[fdo#110210]: https://bugs.freedesktop.org/show_bug.cgi?id=110210

Participating hosts (48 -> 42)
------------------------------

Additional (1): fi-kbl-7500u
Missing (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-y

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

* IGT: IGT_4892 -> IGTPW_2667

CI_DRM_5780: 774f8c588542dda6d73161429cbf1e027789d6ef @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2667: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2667/
IGT_4892: 8ae86621d6fff60b6e20c6b0f9b336785c935b0f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2667/
_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev