On Wed, 2018-10-03 at 11:08 -0500, Rob Herring wrote:
> On Wed, Oct 3, 2018 at 8:22 AM Lubomir Rintel <lkundrak@xxxxx> wrote:
> > If none of the markers bears type information we can't just prepend a new
> > one. Otherwise two subsequent markers will begin at offset 0, blowing up
> > the chunk_len calculation a few lines after.
> >
> > A particular case where this causes havoc is a string property in a tree
> > generated from "fs" input. A string property ends up with a TYPE_NULL
>
> You mean TYPE_NONE?
Yes.
> > marker. Upon output, in write_propval(), we correctly guess it looks
> > like a TYPE_STRING. With the dummy marker prepended, We'd and up with two
> > markers:
> >
> > { type = TYPE_STRING, offset = 0 }
> > { type = TYPE_NONE, offset = 0 }
> >
> > The chunk_len for the fist marker will end up being calculated as 0.
> > This blows up later on because a zero-length string should never happen
> > because it lacks the terminating \0 (valgrind catches this).
>
> I'm slightly confused as to when this is broken. This sounds like the
> output from fs input is completely broken for all strings, but I don't
> think that is true given we've had reports of only minor issues.
Yes, it's indeed broken for all strings. On my system:
$ ./dtc -I fs -O dts tests/fs/test_tree1 |grep '"'
compatible = """test_tree1";
prop-str = """hello world";
compatible = """subsubnode2\0subsubnode";
compatible = """subnode1";
compatible = """subsubnode1\0subsubnode";
placeholder = """this is a placeholder string\0string2";
Note the extra "". But also:
$ valgrind ./dtc -I fs -O dts tests/fs/test_tree1 |grep '"'
==178070== Memcheck, a memory error detector
==178070== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et
al.
==178070== Using Valgrind-3.13.0 and LibVEX; rerun with -h for
copyright info
==178070== Command: ./dtc -I fs -O dts tests/fs/test_tree1
==178070==
==178070== Invalid read of size 1
==178070== at 0x40A547: write_propval_string (treesource.c:67)
==178070== by 0x40A547: write_propval (treesource.c:259)
==178070== by 0x40A547: write_tree_source_node (treesource.c:294)
==178070== by 0x402AC5: main (dtc.c:357)
==178070== Address 0x4a8447f is 1 bytes before a block of size 11
alloc'd
==178070== at 0x4838748: malloc (vg_replace_malloc.c:298)
==178070== by 0x483A9AF: realloc (vg_replace_malloc.c:785)
==178070== by 0x405FCB: xrealloc (util.h:64)
==178070== by 0x405FCB: data_grow_for (data.c:54)
==178070== by 0x40661A: data_copy_file (data.c:107)
==178070== by 0x40833F: read_fstree (fstree.c:61)
==178070== by 0x4083D4: dt_from_fs (fstree.c:86)
==178070== by 0x40294E: main (dtc.c:308)
==178070==
==178070== Invalid read of size 1
==178070== at 0x40A547: write_propval_string (treesource.c:67)
==178070== by 0x40A547: write_propval (treesource.c:259)
==178070== by 0x40A547: write_tree_source_node (treesource.c:294)
==178070== by 0x40A74A: write_tree_source_node (treesource.c:298)
==178070== by 0x40A74A: write_tree_source_node (treesource.c:298)
==178070== by 0x402AC5: main (dtc.c:357)
==178070== Address 0x4a999ff is 1 bytes before a block of size 23
alloc'd
==178070== at 0x4838748: malloc (vg_replace_malloc.c:298)
==178070== by 0x483A9AF: realloc (vg_replace_malloc.c:785)
==178070== by 0x405FCB: xrealloc (util.h:64)
==178070== by 0x405FCB: data_grow_for (data.c:54)
==178070== by 0x40661A: data_copy_file (data.c:107)
==178070== by 0x40833F: read_fstree (fstree.c:61)
==178070== by 0x40838B: read_fstree (fstree.c:70)
==178070== by 0x40838B: read_fstree (fstree.c:70)
==178070== by 0x4083D4: dt_from_fs (fstree.c:86)
==178070== by 0x40294E: main (dtc.c:308)
==178070==
==178070== Invalid read of size 1
==178070== at 0x40A547: write_propval_string (treesource.c:67)
==178070== by 0x40A547: write_propval (treesource.c:259)
==178070== by 0x40A547: write_tree_source_node (treesource.c:294)
==178070== by 0x40A74A: write_tree_source_node (treesource.c:298)
==178070== by 0x402AC5: main (dtc.c:357)
==178070== Address 0x4ab578f is 1 bytes before a block of size 9
alloc'd
==178070== at 0x4838748: malloc (vg_replace_malloc.c:298)
==178070== by 0x483A9AF: realloc (vg_replace_malloc.c:785)
==178070== by 0x405FCB: xrealloc (util.h:64)
==178070== by 0x405FCB: data_grow_for (data.c:54)
==178070== by 0x40661A: data_copy_file (data.c:107)
==178070== by 0x40833F: read_fstree (fstree.c:61)
==178070== by 0x40838B: read_fstree (fstree.c:70)
==178070== by 0x4083D4: dt_from_fs (fstree.c:86)
==178070== by 0x40294E: main (dtc.c:308)
>
> > I'm also adding an assertion to write_propval_string() so that such
> > cases won't go unnoticed in future and a test case.
> >
> > Signed-off-by: Lubomir Rintel <lkundrak@xxxxx>
> > ---
> >
> > Changes since v1:
> > - Oops, forgot to git add tests/fs_tree1.dts. Added now.
> >
> > tests/fs_tree1.dts | 43 +++++++++++++++++++++++++++++++++++++++++++
> > tests/run_tests.sh | 14 ++++++++++++++
> > treesource.c | 3 ++-
> > 3 files changed, 59 insertions(+), 1 deletion(-)
> > create mode 100644 tests/fs_tree1.dts
> >
> > diff --git a/tests/fs_tree1.dts b/tests/fs_tree1.dts
> > new file mode 100644
> > index 0000000..aeae436
> > --- /dev/null
> > +++ b/tests/fs_tree1.dts
> > @@ -0,0 +1,43 @@
> > +/dts-v1/;
> > +
> > +/ {
> > + compatible = "test_tree1";
> > + #address-cells = <0x01>;
> > + #size-cells = <0x00>;
> > + prop-str = "hello world";
> > + prop-int = <0xdeadbeef>;
> > + prop-int64 = <0xdeadbeef 0x1abcdef>;
> > +
> > + subnode@2 {
> > + #address-cells = <0x01>;
> > + linux,phandle = <0x2000>;
> > + #size-cells = <0x00>;
> > + reg = <0x02>;
> > + prop-int = <0x75bcd15>;
> > +
> > + subsubnode@0 {
> > + compatible = "subsubnode2\0subsubnode";
>
> The fs input having '\0' output is a bug BTW. I need to respin my fix,
> but these 2 patches will collide.
>
> > + phandle = <0x2001>;
> > + reg = <0x00>;
> > + prop-int = <0x75bcd15>;
> > + };
> > +
> > + ss2 {
> > + };
> > + };
> > +
> > + subnode@1 {
> > + compatible = "subnode1";
> > + reg = <0x01>;
> > + prop-int = <0xdeadbeef>;
> > +
> > + ss1 {
> > + };
> > +
> > + subsubnode {
> > + compatible = "subsubnode1\0subsubnode";
> > + placeholder = "this is a placeholder string\0string2";
> > + prop-int = <0xdeadbeef>;
> > + };
> > + };
> > +};
> > diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> > index 0e9f345..f8cc481 100755
> > --- a/tests/run_tests.sh
> > +++ b/tests/run_tests.sh
> > @@ -137,6 +137,19 @@ check_align () {
> > )
> > }
> >
> > +# $1: first file
> > +# $2: second file
> > +files_equal () {
> > + shorten_echo "files_equal $@: "
> > + (
> > + if diff -q $1 $2; then
> > + PASS
> > + else
> > + FAIL "File $1 differs from $2"
> > + fi
> > + )
> > +}
> > +
> > run_dtc_test () {
> > printf "dtc $*: "
> > base_run_test wrap_test $VALGRIND $DTC "$@"
> > @@ -460,6 +473,7 @@ libfdt_tests () {
> > run_dtc_test -I fs -O dts -o fs.test_tree1.test.dts $FSBASE/test_tree1
> > run_dtc_test -I fs -O dtb -o fs.test_tree1.test.dtb $FSBASE/test_tree1
> > run_test dtbs_equal_unordered -m fs.test_tree1.test.dtb test_tree1.dtb
> > + base_run_test files_equal fs_tree1.dts fs.test_tree1.test.dts
> >
> > # check full tests
> > for good in test_tree1.dtb; do
> > diff --git a/treesource.c b/treesource.c
> > index c1fdb86..eabb929 100644
> > --- a/treesource.c
> > +++ b/treesource.c
> > @@ -64,6 +64,7 @@ static bool isstring(char c)
> > static void write_propval_string(FILE *f, const char *s, size_t len)
> > {
> > const char *end = s + len - 1;
> > + assert(len > 0);
>
> This can and will happen with REF_PATH markers. See "Fix dts output
> with a REF_PATH marker" patch which David said he applied, but looks
> like he hasn't pushed out yet.
>
> > assert(*end == '\0');
> >
> > fprintf(f, "\"");
> > @@ -221,7 +222,7 @@ static void write_propval(FILE *f, struct property *prop)
> > if (!next_type_marker(m)) {
> > /* data type information missing, need to guess */
> > dummy_marker.type = guess_value_type(prop);
> > - dummy_marker.next = prop->val.markers;
> > + dummy_marker.next = NULL;
> > dummy_marker.offset = 0;
> > dummy_marker.ref = NULL;
> > m = &dummy_marker;
I'm now going to wait until the \0 fix and David's patch you mentioned
lands before I send out a v3.
Thanks
Lubo
