Fix segfault in DTC

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

Quoted 2 lines · Markus Mayer

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.

Quoted 6 lines · Will Deacon

Quoted 2 lines · Markus Mayer

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.

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}

Quoted 15 lines · 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)

Quoted 13 lines

$ 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.

Quoted 30 lines · David Gibson

Quoted 15 lines · 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)

Quoted 13 lines

$ 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}

Quoted 2 lines

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.

Quoted 10 lines · Markus Mayer

Quoted 3 lines · David Gibson

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"

Quoted 3 lines

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.

Quoted 11 lines · David Gibson

Quoted 10 lines · Markus Mayer

Quoted 3 lines · David Gibson

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"

Quoted 7 lines

Quoted 3 lines

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.

Quoted 1 line

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

Yeah, seems like the kernel DTC is quite old.

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

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

Which is the icky step. Again, thank you.

Quoted 5 lines

... 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. :-)

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 17 lines · Jon Loeliger

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

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

Which is the icky step. Again, thank you.

Quoted 5 lines

... 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.

Quoted 1 line

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.

Quoted 18 lines · Stephen Warren

Quoted 17 lines · Jon Loeliger

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

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

Which is the icky step. Again, thank you.

Quoted 5 lines

... 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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 24 lines · David Gibson

Quoted 22 lines · Stephen Warren

Quoted 11 lines · Jon Loeliger

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

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

Which is the icky step. Again, thank you.

Quoted 7 lines

Quoted 5 lines

... 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.

Quoted 1 line

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.

Quoted 27 lines · Stephen Warren

Quoted 25 lines · David Gibson

Quoted 18 lines · Stephen Warren

Quoted 17 lines · Jon Loeliger

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

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

Which is the icky step. Again, thank you.

Quoted 5 lines

... 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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 28 lines · David Gibson

Quoted 27 lines · Stephen Warren

Quoted 25 lines · David Gibson

Quoted 18 lines · Stephen Warren

Quoted 17 lines · Jon Loeliger

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

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

Which is the icky step. Again, thank you.

Quoted 5 lines

... 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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 38 lines · Stephen Warren

Quoted 28 lines · David Gibson

Quoted 27 lines · Stephen Warren

Quoted 25 lines · David Gibson

Quoted 18 lines · Stephen Warren

Quoted 17 lines · Jon Loeliger

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

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

Which is the icky step. Again, thank you.

Quoted 5 lines

... 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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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".

Quoted 46 lines · David Gibson

Quoted 44 lines · Stephen Warren

Quoted 28 lines · David Gibson

Quoted 27 lines · Stephen Warren

Quoted 25 lines · David Gibson

Quoted 18 lines · Stephen Warren

Quoted 17 lines · Jon Loeliger

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

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

Which is the icky step. Again, thank you.

Quoted 5 lines

... 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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 11 lines

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.

Quoted 3 lines

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...