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?
> 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.
> 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;
> --
> 2.19.0
>
