* [igt-dev] [PATCH i-g-t v3] tests/testdisplay: fix heap overflow
@ 2019-03-20 11:48 Simon Ser
2019-03-20 12:00 ` Chris Wilson
2019-03-20 13:57 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/testdisplay: fix heap overflow (rev2) Patchwork
0 siblings, 2 replies; 7+ messages in thread
From: Simon Ser @ 2019-03-20 11:48 UTC (permalink / raw)
To: igt-dev
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 <simon.ser@intel.com>
---
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(-)
diff --git a/tests/testdisplay.c b/tests/testdisplay.c
index b3657264..67d1b68a 100644
--- a/tests/testdisplay.c
+++ b/tests/testdisplay.c
@@ -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) {
--
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
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [igt-dev] [PATCH i-g-t v3] tests/testdisplay: fix heap overflow
2019-03-20 11:48 [igt-dev] [PATCH i-g-t v3] tests/testdisplay: fix heap overflow Simon Ser
@ 2019-03-20 12:00 ` Chris Wilson
2019-03-20 12:17 ` Petri Latvala
2019-03-20 13:57 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/testdisplay: fix heap overflow (rev2) Patchwork
1 sibling, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2019-03-20 12:00 UTC (permalink / raw)
To: Simon Ser, igt-dev
Quoting Simon Ser (2019-03-20 11:48:57)
> + 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).
> - free(exec_path);
> + free(argv0);
Ok, looks like dirname(e) doesn't die with NULL.
> }
>
> 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);
Sigh, I forget about C being unhelpful.
Your call whether that cast is worth it.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v3] tests/testdisplay: fix heap overflow
2019-03-20 12:00 ` Chris Wilson
@ 2019-03-20 12:17 ` Petri Latvala
2019-03-21 10:32 ` Ser, Simon
0 siblings, 1 reply; 7+ messages in thread
From: Petri Latvala @ 2019-03-20 12:17 UTC (permalink / raw)
To: Chris Wilson; +Cc: igt-dev
On Wed, Mar 20, 2019 at 12:00:13PM +0000, Chris Wilson wrote:
> Quoting Simon Ser (2019-03-20 11:48:57)
> > + 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.
--
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v3] tests/testdisplay: fix heap overflow
2019-03-20 12:17 ` Petri Latvala
@ 2019-03-21 10:32 ` Ser, Simon
2019-03-21 10:42 ` Petri Latvala
0 siblings, 1 reply; 7+ messages in thread
From: Ser, Simon @ 2019-03-21 10:32 UTC (permalink / raw)
To: Latvala, Petri, chris@chris-wilson.co.uk; +Cc: igt-dev@lists.freedesktop.org
On Wed, 2019-03-20 at 14:17 +0200, Petri Latvala wrote:
> On Wed, Mar 20, 2019 at 12:00:13PM +0000, Chris Wilson wrote:
> > Quoting Simon Ser (2019-03-20 11:48:57)
> > > + 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.
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
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v3] tests/testdisplay: fix heap overflow
2019-03-21 10:32 ` Ser, Simon
@ 2019-03-21 10:42 ` Petri Latvala
2019-03-25 11:17 ` Petri Latvala
0 siblings, 1 reply; 7+ messages in thread
From: Petri Latvala @ 2019-03-21 10:42 UTC (permalink / raw)
To: Ser, Simon; +Cc: igt-dev@lists.freedesktop.org
On Thu, Mar 21, 2019 at 12:32:24PM +0200, Ser, Simon wrote:
> On Wed, 2019-03-20 at 14:17 +0200, Petri Latvala wrote:
> > On Wed, Mar 20, 2019 at 12:00:13PM +0000, Chris Wilson wrote:
> > > Quoting Simon Ser (2019-03-20 11:48:57)
> > > > + 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.
>
> 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).
--
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v3] tests/testdisplay: fix heap overflow
2019-03-21 10:42 ` Petri Latvala
@ 2019-03-25 11:17 ` Petri Latvala
0 siblings, 0 replies; 7+ messages in thread
From: Petri Latvala @ 2019-03-25 11:17 UTC (permalink / raw)
To: Ser, Simon, chris@chris-wilson.co.uk,
igt-dev@lists.freedesktop.org
On Thu, Mar 21, 2019 at 12:42:13PM +0200, Petri Latvala wrote:
> On Thu, Mar 21, 2019 at 12:32:24PM +0200, Ser, Simon wrote:
> > On Wed, 2019-03-20 at 14:17 +0200, Petri Latvala wrote:
> > > On Wed, Mar 20, 2019 at 12:00:13PM +0000, Chris Wilson wrote:
> > > > Quoting Simon Ser (2019-03-20 11:48:57)
> > > > > + 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.
> >
> > 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.
--
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 7+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for tests/testdisplay: fix heap overflow (rev2)
2019-03-20 11:48 [igt-dev] [PATCH i-g-t v3] tests/testdisplay: fix heap overflow Simon Ser
2019-03-20 12:00 ` Chris Wilson
@ 2019-03-20 13:57 ` Patchwork
1 sibling, 0 replies; 7+ messages in thread
From: Patchwork @ 2019-03-20 13:57 UTC (permalink / raw)
To: Simon Ser; +Cc: igt-dev
== 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
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-25 11:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-20 11:48 [igt-dev] [PATCH i-g-t v3] tests/testdisplay: fix heap overflow Simon Ser
2019-03-20 12:00 ` Chris Wilson
2019-03-20 12:17 ` Petri Latvala
2019-03-21 10:32 ` Ser, Simon
2019-03-21 10:42 ` Petri Latvala
2019-03-25 11:17 ` Petri Latvala
2019-03-20 13:57 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/testdisplay: fix heap overflow (rev2) Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox