[PATCH 0/1] Fix segfault in DTC

[PATCH 0/1] Fix segfault in DTC

[Markus Mayer]

Please let me know if there is a different place this patch should be sent
to. Thanks.

Markus Mayer (1):
  Fix segfault in dtc when empty input file is given.

 scripts/dtc/treesource.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

-- 
1.7.5.4

[PATCH 1/1] Fix segfault in DTC

[Markus Mayer]

Prior to this change, an empty input file would cause a segfault, because
yylloc had never been initialized. There was never any characters for the
lexer to match, so YY_USER_ACTION was never executed before the parse error
was detected.

When the parser printed the error message, it tried to include the name of
the file, but the structure holding the file name (yylloc.file, referenced
as pos->file) had never been initialized.

Without the fix:

$ ./dtc /dev/null
DTC: dts->dts  on file "/dev/null"
Segmentation fault (core dumped)

$ gdb dtc core
Program terminated with signal 11, Segmentation fault.
    at scripts/dtc/srcpos.c:194
194			fname = pos->file->name;
(gdb) bt
    at scripts/dtc/srcpos.c:194
    fmt=0x40d769 "%s", va=0x7fffbf027148) at scripts/dtc/srcpos.c:220
    at scripts/dtc/dtc-parser.tab.c:1920
    at scripts/dtc/treesource.c:38
    at scripts/dtc/dtc.c:203
(gdb) p *pos
$1 = {first_line = 0, first_column = 0, last_line = 0, last_column = 0,
  file = 0x0}

With the fix:

$ ./dtc /dev/null
DTC: dts->dts  on file "/dev/null"
Error: /dev/null:1.1 syntax error
FATAL ERROR: Unable to parse input tree

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 scripts/dtc/treesource.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/scripts/dtc/treesource.c b/scripts/dtc/treesource.c
index c09aafa..b461b88 100644
--- a/scripts/dtc/treesource.c
+++ b/scripts/dtc/treesource.c
@@ -29,11 +29,15 @@ int treesource_error;
 
 struct boot_info *dt_from_source(const char *fname)
 {
+	extern YYLTYPE yylloc;
+
 	the_boot_info = NULL;
 	treesource_error = 0;
 
 	srcfile_push(fname);
 	yyin = current_srcfile->f;
+	/* Initialize yylloc->file to avoid segfault on empty input */
+	srcpos_update(&yylloc, NULL, 0);
 
 	if (yyparse() != 0)
 		die("Unable to parse input tree\n");
-- 
1.7.5.4

[PATCH 0/1] Fix segfault in DTC

[Will Deacon]

On Mon, Sep 24, 2012 at 11:48:04PM +0100, Markus Mayer wrote:
> Please let me know if there is a different place this patch should be sent
> to. Thanks.

You could probably CC devicetree-discuss and also David Gibson for patches
against dtc.

Will

[PATCH 0/1] Fix segfault in DTC

[Stephen Warren]

On 09/25/2012 05:07 AM, Will Deacon wrote:
> On Mon, Sep 24, 2012 at 11:48:04PM +0100, Markus Mayer wrote:
>> Please let me know if there is a different place this patch should be sent
>> to. Thanks.
> 
> You could probably CC devicetree-discuss and also David Gibson for patches
> against dtc.

And Jon Loeliger.

[PATCH 0/1] Fix segfault in DTC

[Markus Mayer]

On 25/09/2012 08:44, Stephen Warren wrote:
> On 09/25/2012 05:07 AM, Will Deacon wrote:
>> On Mon, Sep 24, 2012 at 11:48:04PM +0100, Markus Mayer wrote:
>>> Please let me know if there is a different place this patch should be sent
>>> to. Thanks.
>>
>> You could probably CC devicetree-discuss and also David Gibson for patches
>> against dtc.
>
> And Jon Loeliger.

Thanks, guys.

-Markus

[PATCH 0/1] Fix segfault in DTC

[Markus Mayer]

As suggested on linux-arm-kernel, I am copying devicetree-discuss, as well
as David and Jon on this thread. Please let me know your thoughts.

Regards,
-Markus

Markus Mayer (1):
  Fix segfault in dtc when empty input file is given.

 scripts/dtc/treesource.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

-- 
1.7.5.4

[PATCH 1/1] Fix segfault in DTC

[Markus Mayer]

Prior to this change, an empty input file would cause a segfault, because
yylloc had never been initialized. There was never any characters for the
lexer to match, so YY_USER_ACTION was never executed before the parse error
was detected.

When the parser printed the error message, it tried to include the name of
the file, but the structure holding the file name (yylloc.file, referenced
as pos->file) had never been initialized.

Without the fix:

$ ./dtc /dev/null
DTC: dts->dts  on file "/dev/null"
Segmentation fault (core dumped)

$ gdb dtc core
Program terminated with signal 11, Segmentation fault.
    at scripts/dtc/srcpos.c:194
194			fname = pos->file->name;
(gdb) bt
    at scripts/dtc/srcpos.c:194
    fmt=0x40d769 "%s", va=0x7fffbf027148) at scripts/dtc/srcpos.c:220
    at scripts/dtc/dtc-parser.tab.c:1920
    at scripts/dtc/treesource.c:38
    at scripts/dtc/dtc.c:203
(gdb) p *pos
$1 = {first_line = 0, first_column = 0, last_line = 0, last_column = 0,
  file = 0x0}

With the fix:

$ ./dtc /dev/null
DTC: dts->dts  on file "/dev/null"
Error: /dev/null:1.1 syntax error
FATAL ERROR: Unable to parse input tree

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 scripts/dtc/treesource.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/scripts/dtc/treesource.c b/scripts/dtc/treesource.c
index c09aafa..b461b88 100644
--- a/scripts/dtc/treesource.c
+++ b/scripts/dtc/treesource.c
@@ -29,11 +29,15 @@ int treesource_error;
 
 struct boot_info *dt_from_source(const char *fname)
 {
+	extern YYLTYPE yylloc;
+
 	the_boot_info = NULL;
 	treesource_error = 0;
 
 	srcfile_push(fname);
 	yyin = current_srcfile->f;
+	/* Initialize yylloc->file to avoid segfault on empty input */
+	srcpos_update(&yylloc, NULL, 0);
 
 	if (yyparse() != 0)
 		die("Unable to parse input tree\n");
-- 
1.7.5.4

[PATCH 1/1] Fix segfault in DTC

[David Gibson]

On Tue, Sep 25, 2012 at 10:58:09AM -0700, Markus Mayer wrote:
> Prior to this change, an empty input file would cause a segfault, because
> yylloc had never been initialized. There was never any characters for the
> lexer to match, so YY_USER_ACTION was never executed before the parse error
> was detected.
> 
> When the parser printed the error message, it tried to include the name of
> the file, but the structure holding the file name (yylloc.file, referenced
> as pos->file) had never been initialized.
> 
> Without the fix:
> 
> $ ./dtc /dev/null
> DTC: dts->dts  on file "/dev/null"
> Segmentation fault (core dumped)
> 
> $ gdb dtc core
> Program terminated with signal 11, Segmentation fault.
>     at scripts/dtc/srcpos.c:194
> 194			fname = pos->file->name;
> (gdb) bt
>     at scripts/dtc/srcpos.c:194
>     fmt=0x40d769 "%s", va=0x7fffbf027148) at scripts/dtc/srcpos.c:220
>     at scripts/dtc/dtc-parser.tab.c:1920
>     at scripts/dtc/treesource.c:38
>     at scripts/dtc/dtc.c:203
> (gdb) p *pos
> $1 = {first_line = 0, first_column = 0, last_line = 0, last_column = 0,
>   file = 0x0}

Which dtc version did you observe this with?  I'm unable to reproduce
the SEGV with current git.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 1/1] Fix segfault in DTC

[Markus Mayer]

On 25/09/2012 16:30, David Gibson wrote:
> On Tue, Sep 25, 2012 at 10:58:09AM -0700, Markus Mayer wrote:
>> Prior to this change, an empty input file would cause a segfault, because
>> yylloc had never been initialized. There was never any characters for the
>> lexer to match, so YY_USER_ACTION was never executed before the parse error
>> was detected.
>>
>> When the parser printed the error message, it tried to include the name of
>> the file, but the structure holding the file name (yylloc.file, referenced
>> as pos->file) had never been initialized.
>>
>> Without the fix:
>>
>> $ ./dtc /dev/null
>> DTC: dts->dts  on file "/dev/null"
>> Segmentation fault (core dumped)
>>
>> $ gdb dtc core
>> Program terminated with signal 11, Segmentation fault.
>>      at scripts/dtc/srcpos.c:194
>> 194			fname = pos->file->name;
>> (gdb) bt
>>      at scripts/dtc/srcpos.c:194
>>      fmt=0x40d769 "%s", va=0x7fffbf027148) at scripts/dtc/srcpos.c:220
>>      at scripts/dtc/dtc-parser.tab.c:1920
>>      at scripts/dtc/treesource.c:38
>>      at scripts/dtc/dtc.c:203
>> (gdb) p *pos
>> $1 = {first_line = 0, first_column = 0, last_line = 0, last_column = 0,
>>    file = 0x0}
>
> Which dtc version did you observe this with?  I'm unable to reproduce
> the SEGV with current git.

That was

$ cat version_gen.h
#define DTC_VERSION "DTC 1.2.0-g37c0b6a0"

from git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git. 
In other words, the DTC used by the Linux 3.6-rc series.

Regards,
-Markus

[PATCH 1/1] Fix segfault in DTC

[David Gibson]

On Tue, Sep 25, 2012 at 04:51:47PM -0700, Markus Mayer wrote:
> On 25/09/2012 16:30, David Gibson wrote:
> >On Tue, Sep 25, 2012 at 10:58:09AM -0700, Markus Mayer wrote:
> >>Prior to this change, an empty input file would cause a segfault, because
> >>yylloc had never been initialized. There was never any characters for the
> >>lexer to match, so YY_USER_ACTION was never executed before the parse error
> >>was detected.
> >>
> >>When the parser printed the error message, it tried to include the name of
> >>the file, but the structure holding the file name (yylloc.file, referenced
> >>as pos->file) had never been initialized.
> >>
> >>Without the fix:
> >>
> >>$ ./dtc /dev/null
> >>DTC: dts->dts  on file "/dev/null"
> >>Segmentation fault (core dumped)
> >>
> >>$ gdb dtc core
> >>Program terminated with signal 11, Segmentation fault.
> >>     at scripts/dtc/srcpos.c:194
> >>194			fname = pos->file->name;
> >>(gdb) bt
> >>     at scripts/dtc/srcpos.c:194
> >>     fmt=0x40d769 "%s", va=0x7fffbf027148) at scripts/dtc/srcpos.c:220
> >>     at scripts/dtc/dtc-parser.tab.c:1920
> >>     at scripts/dtc/treesource.c:38
> >>     at scripts/dtc/dtc.c:203
> >>(gdb) p *pos
> >>$1 = {first_line = 0, first_column = 0, last_line = 0, last_column = 0,
> >>   file = 0x0}
> >
> >Which dtc version did you observe this with?  I'm unable to reproduce
> >the SEGV with current git.
> 
> That was
> 
> $ cat version_gen.h
> #define DTC_VERSION "DTC 1.2.0-g37c0b6a0"
> 
> from
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git. In
> other words, the DTC used by the Linux 3.6-rc series.

Ah, ok.  Patches to dtc should be made against the upstream version at
git://git.jdl.com/software/dtc.git.  In this case the bug was already
fixed there, in commit a6e6c60e3a97a6b3a033cd052bb3740fd53cbf4c.

I think we're overdue to update the kernel copy of dtc from upstream.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 1/1] Fix segfault in DTC

[Markus Mayer]

On 25/09/2012 17:35, David Gibson wrote:
> On Tue, Sep 25, 2012 at 04:51:47PM -0700, Markus Mayer wrote:
>> On 25/09/2012 16:30, David Gibson wrote:
>>>
>>> Which dtc version did you observe this with?  I'm unable to reproduce
>>> the SEGV with current git.
>>
>> That was
>>
>> $ cat version_gen.h
>> #define DTC_VERSION "DTC 1.2.0-g37c0b6a0"
>>
>> from
>> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git. In
>> other words, the DTC used by the Linux 3.6-rc series.
>
> Ah, ok.  Patches to dtc should be made against the upstream version at
> git://git.jdl.com/software/dtc.git.  In this case the bug was already
> fixed there, in commit a6e6c60e3a97a6b3a033cd052bb3740fd53cbf4c.

Ah yes. That makes sense. Thanks for pointing out the commit that fixes 
the issue. I just checked it out.

> I think we're overdue to update the kernel copy of dtc from upstream.

Yeah, seems like the kernel DTC is quite old.

Regards,
-Markus

[PATCH 1/1] Fix segfault in DTC

[Stephen Warren]

On 09/26/2012 10:38 AM, Markus Mayer wrote:
> On 25/09/2012 17:35, David Gibson wrote:
>> On Tue, Sep 25, 2012 at 04:51:47PM -0700, Markus Mayer wrote:
>>> On 25/09/2012 16:30, David Gibson wrote:
>>>>
>>>> Which dtc version did you observe this with?  I'm unable to reproduce
>>>> the SEGV with current git.
>>>
>>> That was
>>>
>>> $ cat version_gen.h
>>> #define DTC_VERSION "DTC 1.2.0-g37c0b6a0"
>>>
>>> from
>>> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git. In
>>> other words, the DTC used by the Linux 3.6-rc series.
>>
>> Ah, ok.  Patches to dtc should be made against the upstream version at
>> git://git.jdl.com/software/dtc.git.  In this case the bug was already
>> fixed there, in commit a6e6c60e3a97a6b3a033cd052bb3740fd53cbf4c.
> 
> Ah yes. That makes sense. Thanks for pointing out the commit that fixes
> the issue. I just checked it out.
> 
>> I think we're overdue to update the kernel copy of dtc from upstream.
> 
> Yeah, seems like the kernel DTC is quite old.

FYI, I'm working on a patch to the kernel to bring in the latest dtc.

I've run a regression test vs. the old dtc in the kernel and found that
some of the PowerPC .dts files don't compile with the new dtc (but did
with the old), all due to non-existent labels/paths being referenced.
I'll try and track down whether this is a regression in dtc, or simply
buggy .dts files that weren't noticed before.

Everything non-PowerPC works fine, and most PowerPC files work fine.
Where compilation succeeds, the md5sum of the .dtb is identical in every
case.

[PATCH 1/1] Fix segfault in DTC

[Jon Loeliger]

> > 
> > Yeah, seems like the kernel DTC is quite old.
> 
> FYI, I'm working on a patch to the kernel to bring in the latest dtc.

Awesome.  Thank you.

> I've run a regression test vs. the old dtc in the kernel ...

Which is the icky step.  Again, thank you.

> ... and found that
> some of the PowerPC .dts files don't compile with the new dtc (but did
> with the old), all due to non-existent labels/paths being referenced.
> I'll try and track down whether this is a regression in dtc, or simply
> buggy .dts files that weren't noticed before.

I think you should just smack the PowerPC guys. :-)

> Everything non-PowerPC works fine, and most PowerPC files work fine.
> Where compilation succeeds, the md5sum of the .dtb is identical in every
> case.

Cool.

Thanks,
jdl

[PATCH 1/1] Fix segfault in DTC

[Stephen Warren]

On 09/28/2012 12:53 PM, Jon Loeliger wrote:
>>>
>>> Yeah, seems like the kernel DTC is quite old.
>>
>> FYI, I'm working on a patch to the kernel to bring in the latest dtc.
> 
> Awesome.  Thank you.
> 
>> I've run a regression test vs. the old dtc in the kernel ...
> 
> Which is the icky step.  Again, thank you.
> 
>> ... and found that
>> some of the PowerPC .dts files don't compile with the new dtc (but did
>> with the old), all due to non-existent labels/paths being referenced.
>> I'll try and track down whether this is a regression in dtc, or simply
>> buggy .dts files that weren't noticed before.
> 
> I think you should just smack the PowerPC guys. :-)

For the record in this thread, it was a regression I introduced into dtc
- the patch I just sent was for this.

>> Everything non-PowerPC works fine, and most PowerPC files work fine.
>> Where compilation succeeds, the md5sum of the .dtb is identical in every
>> case.

... and with that patch, every .dts in the kernel compiles to the same
md5sum before/after.

[PATCH 1/1] Fix segfault in DTC

[Jon Loeliger]

> > 
> > I think you should just smack the PowerPC guys. :-)
> 
> For the record in this thread, it was a regression I introduced into dtc
> - the patch I just sent was for this.

Ah, ok.

I'll apply the DTC patch, but I think you should
still smack the PowerPC guys on general principle. :-)

jdl

[PATCH 1/1] Fix segfault in DTC

[David Gibson]

On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote:
> On 09/28/2012 12:53 PM, Jon Loeliger wrote:
> >>>
> >>> Yeah, seems like the kernel DTC is quite old.
> >>
> >> FYI, I'm working on a patch to the kernel to bring in the latest dtc.
> > 
> > Awesome.  Thank you.
> > 
> >> I've run a regression test vs. the old dtc in the kernel ...
> > 
> > Which is the icky step.  Again, thank you.
> > 
> >> ... and found that
> >> some of the PowerPC .dts files don't compile with the new dtc (but did
> >> with the old), all due to non-existent labels/paths being referenced.
> >> I'll try and track down whether this is a regression in dtc, or simply
> >> buggy .dts files that weren't noticed before.
> > 
> > I think you should just smack the PowerPC guys. :-)
> 
> For the record in this thread, it was a regression I introduced into dtc
> - the patch I just sent was for this.

I would be nice to add a testcase for this regression into dtc.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 1/1] Fix segfault in DTC

[Stephen Warren]

On 09/29/2012 05:53 PM, David Gibson wrote:
> On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote:
>> On 09/28/2012 12:53 PM, Jon Loeliger wrote:
>>>>>
>>>>> Yeah, seems like the kernel DTC is quite old.
>>>>
>>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc.
>>>
>>> Awesome.  Thank you.
>>>
>>>> I've run a regression test vs. the old dtc in the kernel ...
>>>
>>> Which is the icky step.  Again, thank you.
>>>
>>>> ... and found that
>>>> some of the PowerPC .dts files don't compile with the new dtc (but did
>>>> with the old), all due to non-existent labels/paths being referenced.
>>>> I'll try and track down whether this is a regression in dtc, or simply
>>>> buggy .dts files that weren't noticed before.
>>>
>>> I think you should just smack the PowerPC guys. :-)
>>
>> For the record in this thread, it was a regression I introduced into dtc
>> - the patch I just sent was for this.
> 
> I would be nice to add a testcase for this regression into dtc.

The issue here was caused by uninitialized memory, so it would, I think,
be basically impossible to create a test-case that would be guaranteed
to fail because of this; it'd depend on the internal details of the
malloc library and how/when it re-used previously free()d memory blocks.

[PATCH 1/1] Fix segfault in DTC

[David Gibson]

On Sun, Sep 30, 2012 at 11:34:50PM -0600, Stephen Warren wrote:
> On 09/29/2012 05:53 PM, David Gibson wrote:
> > On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote:
> >> On 09/28/2012 12:53 PM, Jon Loeliger wrote:
> >>>>>
> >>>>> Yeah, seems like the kernel DTC is quite old.
> >>>>
> >>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc.
> >>>
> >>> Awesome.  Thank you.
> >>>
> >>>> I've run a regression test vs. the old dtc in the kernel ...
> >>>
> >>> Which is the icky step.  Again, thank you.
> >>>
> >>>> ... and found that
> >>>> some of the PowerPC .dts files don't compile with the new dtc (but did
> >>>> with the old), all due to non-existent labels/paths being referenced.
> >>>> I'll try and track down whether this is a regression in dtc, or simply
> >>>> buggy .dts files that weren't noticed before.
> >>>
> >>> I think you should just smack the PowerPC guys. :-)
> >>
> >> For the record in this thread, it was a regression I introduced into dtc
> >> - the patch I just sent was for this.
> > 
> > I would be nice to add a testcase for this regression into dtc.
> 
> The issue here was caused by uninitialized memory, so it would, I think,
> be basically impossible to create a test-case that would be guaranteed
> to fail because of this; it'd depend on the internal details of the
> malloc library and how/when it re-used previously free()d memory blocks.

It doesn't have to be guaranteed to fail to be useful.  Plus, we
already have the infrastructure to run the tests under valgrind, which
would catch it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 1/1] Fix segfault in DTC

[Stephen Warren]

On 10/01/2012 12:46 AM, David Gibson wrote:
> On Sun, Sep 30, 2012 at 11:34:50PM -0600, Stephen Warren wrote:
>> On 09/29/2012 05:53 PM, David Gibson wrote:
>>> On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote:
>>>> On 09/28/2012 12:53 PM, Jon Loeliger wrote:
>>>>>>>
>>>>>>> Yeah, seems like the kernel DTC is quite old.
>>>>>>
>>>>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc.
>>>>>
>>>>> Awesome.  Thank you.
>>>>>
>>>>>> I've run a regression test vs. the old dtc in the kernel ...
>>>>>
>>>>> Which is the icky step.  Again, thank you.
>>>>>
>>>>>> ... and found that
>>>>>> some of the PowerPC .dts files don't compile with the new dtc (but did
>>>>>> with the old), all due to non-existent labels/paths being referenced.
>>>>>> I'll try and track down whether this is a regression in dtc, or simply
>>>>>> buggy .dts files that weren't noticed before.
>>>>>
>>>>> I think you should just smack the PowerPC guys. :-)
>>>>
>>>> For the record in this thread, it was a regression I introduced into dtc
>>>> - the patch I just sent was for this.
>>>
>>> I would be nice to add a testcase for this regression into dtc.
>>
>> The issue here was caused by uninitialized memory, so it would, I think,
>> be basically impossible to create a test-case that would be guaranteed
>> to fail because of this; it'd depend on the internal details of the
>> malloc library and how/when it re-used previously free()d memory blocks.
> 
> It doesn't have to be guaranteed to fail to be useful.  Plus, we
> already have the infrastructure to run the tests under valgrind, which
> would catch it.

I certainly disagree here; the absolute worst kind of test is one which
gives different results each time it's run, or statically gives
different results to different people. People will either ignore the
test because it's flaky, or it'll end up blaming the wrong person due to
some entirely unrelated and correct change just happening to tickle the
test.

If we were to force any such new test to always run under valgrind, then
hopefully the test would always fail (assuming the test harness triggers
failure if valgrind finds problems). That might be reasonable. However,
if we do this, then I think instead we should just always force the
entire existing test suite to run under valgrind; no new test should be
needed for valgrind to detect this failure (since there was a
read-before-write problem any time a label was used in the existing
code, it just didn't always cause a user-visible issue).

Another alternative might be to LD_PRELOAD a test malloc library with a
known allocation pattern. That would allow a new test for this case to
always fail. However, the allocation pattern of this test malloc library
would have to be carefully tied to the allocation pattern of dtc itself
when processing the new test's .dts file, and that could easily and
legitimately change due to unrelated implementation details of dtc
changing, thus making the maintenance of the test malloc library
painful. So, I don't think this is the way to go.

[PATCH 1/1] Fix segfault in DTC

[David Gibson]

On Mon, Oct 01, 2012 at 10:41:09AM -0600, Stephen Warren wrote:
> On 10/01/2012 12:46 AM, David Gibson wrote:
> > On Sun, Sep 30, 2012 at 11:34:50PM -0600, Stephen Warren wrote:
> >> On 09/29/2012 05:53 PM, David Gibson wrote:
> >>> On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote:
> >>>> On 09/28/2012 12:53 PM, Jon Loeliger wrote:
> >>>>>>>
> >>>>>>> Yeah, seems like the kernel DTC is quite old.
> >>>>>>
> >>>>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc.
> >>>>>
> >>>>> Awesome.  Thank you.
> >>>>>
> >>>>>> I've run a regression test vs. the old dtc in the kernel ...
> >>>>>
> >>>>> Which is the icky step.  Again, thank you.
> >>>>>
> >>>>>> ... and found that
> >>>>>> some of the PowerPC .dts files don't compile with the new dtc (but did
> >>>>>> with the old), all due to non-existent labels/paths being referenced.
> >>>>>> I'll try and track down whether this is a regression in dtc, or simply
> >>>>>> buggy .dts files that weren't noticed before.
> >>>>>
> >>>>> I think you should just smack the PowerPC guys. :-)
> >>>>
> >>>> For the record in this thread, it was a regression I introduced into dtc
> >>>> - the patch I just sent was for this.
> >>>
> >>> I would be nice to add a testcase for this regression into dtc.
> >>
> >> The issue here was caused by uninitialized memory, so it would, I think,
> >> be basically impossible to create a test-case that would be guaranteed
> >> to fail because of this; it'd depend on the internal details of the
> >> malloc library and how/when it re-used previously free()d memory blocks.
> > 
> > It doesn't have to be guaranteed to fail to be useful.  Plus, we
> > already have the infrastructure to run the tests under valgrind, which
> > would catch it.
> 
> I certainly disagree here; the absolute worst kind of test is one which
> gives different results each time it's run, or statically gives
> different results to different people. People will either ignore the
> test because it's flaky, or it'll end up blaming the wrong person due to
> some entirely unrelated and correct change just happening to tickle the
> test.

I'd agree 100% if the test could give false failures.  But in this
case it can only give false passes.  If the test fails there is a bug
*somewhere*, even if it's not actually in whatever changed last.  The
test framework actually has a "PASS (inconclusive)" result for exactly
this sort of case.

> If we were to force any such new test to always run under valgrind, then
> hopefully the test would always fail (assuming the test harness triggers
> failure if valgrind finds problems).

Aside: it's supposed to;  if it doesn't, that's a bug.  You can try it
easily enough with "make checkm".

> That might be reasonable. However,
> if we do this, then I think instead we should just always force the
> entire existing test suite to run under valgrind; no new test should be
> needed for valgrind to detect this failure (since there was a
> read-before-write problem any time a label was used in the existing
> code, it just didn't always cause a user-visible issue).

Hrm.  I don't want to run the whole testsuite under valgrind by
default for a couple of reasons.  First, it's much, much slower, and
second you need to have valgrind installed.  Both these would
discourage people from running the testsuite so frequently, and even
without valgrind it's very, very useful.

I guess we could make the new one a "valgrind only" test, that's we
only bother to execute when the valgrind options are enabled.

> Another alternative might be to LD_PRELOAD a test malloc library with a
> known allocation pattern. That would allow a new test for this case to
> always fail. However, the allocation pattern of this test malloc library
> would have to be carefully tied to the allocation pattern of dtc itself
> when processing the new test's .dts file, and that could easily and
> legitimately change due to unrelated implementation details of dtc
> changing, thus making the maintenance of the test malloc library
> painful. So, I don't think this is the way to go.

Hrm.  A malloc() with a "known buggy" allocation pattern would be
impractical, I agree.  But it occurred to me that a much simpler
malloc() wrapper which filled each allocated block with garbage
(non-zero) data before returning it would be sufficient in this case.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 1/1] Fix segfault in DTC

[Stephen Warren]

On 10/01/2012 06:08 PM, David Gibson wrote:
> On Mon, Oct 01, 2012 at 10:41:09AM -0600, Stephen Warren wrote:
>> On 10/01/2012 12:46 AM, David Gibson wrote:
>>> On Sun, Sep 30, 2012 at 11:34:50PM -0600, Stephen Warren wrote:
>>>> On 09/29/2012 05:53 PM, David Gibson wrote:
>>>>> On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote:
>>>>>> On 09/28/2012 12:53 PM, Jon Loeliger wrote:
>>>>>>>>>
>>>>>>>>> Yeah, seems like the kernel DTC is quite old.
>>>>>>>>
>>>>>>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc.
>>>>>>>
>>>>>>> Awesome.  Thank you.
>>>>>>>
>>>>>>>> I've run a regression test vs. the old dtc in the kernel ...
>>>>>>>
>>>>>>> Which is the icky step.  Again, thank you.
>>>>>>>
>>>>>>>> ... and found that
>>>>>>>> some of the PowerPC .dts files don't compile with the new dtc (but did
>>>>>>>> with the old), all due to non-existent labels/paths being referenced.
>>>>>>>> I'll try and track down whether this is a regression in dtc, or simply
>>>>>>>> buggy .dts files that weren't noticed before.
>>>>>>>
>>>>>>> I think you should just smack the PowerPC guys. :-)
>>>>>>
>>>>>> For the record in this thread, it was a regression I introduced into dtc
>>>>>> - the patch I just sent was for this.
>>>>>
>>>>> I would be nice to add a testcase for this regression into dtc.
>>>>
>>>> The issue here was caused by uninitialized memory, so it would, I think,
>>>> be basically impossible to create a test-case that would be guaranteed
>>>> to fail because of this; it'd depend on the internal details of the
>>>> malloc library and how/when it re-used previously free()d memory blocks.
>>>
>>> It doesn't have to be guaranteed to fail to be useful.  Plus, we
>>> already have the infrastructure to run the tests under valgrind, which
>>> would catch it.
>>
>> I certainly disagree here; the absolute worst kind of test is one which
>> gives different results each time it's run, or statically gives
>> different results to different people. People will either ignore the
>> test because it's flaky, or it'll end up blaming the wrong person due to
>> some entirely unrelated and correct change just happening to tickle the
>> test.
> 
> I'd agree 100% if the test could give false failures.  But in this
> case it can only give false passes.

That is true. I still dislike flaky tests irrespective of
false-{negative,positive} though.

> If the test fails there is a bug
> *somewhere*, even if it's not actually in whatever changed last.  The
> test framework actually has a "PASS (inconclusive)" result for exactly
> this sort of case.
> 
>> If we were to force any such new test to always run under valgrind, then
>> hopefully the test would always fail (assuming the test harness triggers
>> failure if valgrind finds problems).
> 
> Aside: it's supposed to;  if it doesn't, that's a bug.  You can try it
> easily enough with "make checkm".

Ah, I didn't know about that make target (or even "make check"; I'd
always run run_tests.sh manually).

Incidentally, before commit 317a5d9 "dtc: zero out new label objects"
the following two tests fail under make checkm:

dtc -I dts -O dtb -o multilabel.test.dtb multilabel.dts:        FAIL
Returned error code 126

dtc -I dts -O dtb -o multilabel_merge.test.dtb multilabel_merge.dts:
FAIL Returned error code 126

However, they pass at/after that commit.

Equally, those failures were introduced with commit 45013d8 "dtc: Add
ability to delete nodes and properties", which is exactly what I'd
expect given the fix was a fix for that commit.

So, it seems like we already have tests that catch this problem. Do we
need to do anything given that?

I am slightly surprised that the problem didn't cause all tests to fail
make checkm though (I'd expect any usage of a label to trigger the
problem); I'll have to think about why some more...

[PATCH 1/1] Fix segfault in DTC

[David Gibson]

On Wed, Oct 03, 2012 at 03:33:30PM -0600, Stephen Warren wrote:
> On 10/01/2012 06:08 PM, David Gibson wrote:
> > On Mon, Oct 01, 2012 at 10:41:09AM -0600, Stephen Warren wrote:
> >> On 10/01/2012 12:46 AM, David Gibson wrote:
> >>> On Sun, Sep 30, 2012 at 11:34:50PM -0600, Stephen Warren wrote:
> >>>> On 09/29/2012 05:53 PM, David Gibson wrote:
> >>>>> On Fri, Sep 28, 2012 at 01:05:33PM -0600, Stephen Warren wrote:
> >>>>>> On 09/28/2012 12:53 PM, Jon Loeliger wrote:
> >>>>>>>>>
> >>>>>>>>> Yeah, seems like the kernel DTC is quite old.
> >>>>>>>>
> >>>>>>>> FYI, I'm working on a patch to the kernel to bring in the latest dtc.
> >>>>>>>
> >>>>>>> Awesome.  Thank you.
> >>>>>>>
> >>>>>>>> I've run a regression test vs. the old dtc in the kernel ...
> >>>>>>>
> >>>>>>> Which is the icky step.  Again, thank you.
> >>>>>>>
> >>>>>>>> ... and found that
> >>>>>>>> some of the PowerPC .dts files don't compile with the new dtc (but did
> >>>>>>>> with the old), all due to non-existent labels/paths being referenced.
> >>>>>>>> I'll try and track down whether this is a regression in dtc, or simply
> >>>>>>>> buggy .dts files that weren't noticed before.
> >>>>>>>
> >>>>>>> I think you should just smack the PowerPC guys. :-)
> >>>>>>
> >>>>>> For the record in this thread, it was a regression I introduced into dtc
> >>>>>> - the patch I just sent was for this.
> >>>>>
> >>>>> I would be nice to add a testcase for this regression into dtc.
> >>>>
> >>>> The issue here was caused by uninitialized memory, so it would, I think,
> >>>> be basically impossible to create a test-case that would be guaranteed
> >>>> to fail because of this; it'd depend on the internal details of the
> >>>> malloc library and how/when it re-used previously free()d memory blocks.
> >>>
> >>> It doesn't have to be guaranteed to fail to be useful.  Plus, we
> >>> already have the infrastructure to run the tests under valgrind, which
> >>> would catch it.
> >>
> >> I certainly disagree here; the absolute worst kind of test is one which
> >> gives different results each time it's run, or statically gives
> >> different results to different people. People will either ignore the
> >> test because it's flaky, or it'll end up blaming the wrong person due to
> >> some entirely unrelated and correct change just happening to tickle the
> >> test.
> > 
> > I'd agree 100% if the test could give false failures.  But in this
> > case it can only give false passes.
> 
> That is true. I still dislike flaky tests irrespective of
> false-{negative,positive} though.

I don't love it, but I think a test which can generate false-negatives
is better than no test.

> > If the test fails there is a bug
> > *somewhere*, even if it's not actually in whatever changed last.  The
> > test framework actually has a "PASS (inconclusive)" result for exactly
> > this sort of case.
> > 
> >> If we were to force any such new test to always run under valgrind, then
> >> hopefully the test would always fail (assuming the test harness triggers
> >> failure if valgrind finds problems).
> > 
> > Aside: it's supposed to;  if it doesn't, that's a bug.  You can try it
> > easily enough with "make checkm".
> 
> Ah, I didn't know about that make target (or even "make check"; I'd
> always run run_tests.sh manually).
> 
> Incidentally, before commit 317a5d9 "dtc: zero out new label objects"
> the following two tests fail under make checkm:
> 
> dtc -I dts -O dtb -o multilabel.test.dtb multilabel.dts:        FAIL
> Returned error code 126
> 
> dtc -I dts -O dtb -o multilabel_merge.test.dtb multilabel_merge.dts:
> FAIL Returned error code 126
> 
> However, they pass at/after that commit.

Ah, interesting.  Goes to show that neither Jon nor I runs the
valgrind check as often as we probably should.

> Equally, those failures were introduced with commit 45013d8 "dtc: Add
> ability to delete nodes and properties", which is exactly what I'd
> expect given the fix was a fix for that commit.
> 
> So, it seems like we already have tests that catch this problem. Do we
> need to do anything given that?

Hm, I guess not.

> I am slightly surprised that the problem didn't cause all tests to fail
> make checkm though (I'd expect any usage of a label to trigger the
> problem); I'll have to think about why some more...

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson