On Thu, Aug 30, 2018 at 12:01:59PM +0200, Greg Kurz wrote:
> Since commit 32b9c6130762 "Preserve datatype markers when emitting dts
> format", we no longer try to guess the value type. Instead, we reuse
> the type of the datatype markers when they are present, if the type
> is either TYPE_UINT* or TYPE_STRING.
>
> This causes 'dtc -I fs' to crash:
>
> Starting program: /root/dtc -q -f -O dts -I fs /proc/device-tree
> /dts-v1/;
>
> / {
>
> Program received signal SIGSEGV, Segmentation fault.
> __strlen_power8 () at ../sysdeps/powerpc/powerpc64/power8/strlen.S:47
> 47 ld r12,0(r4) /* Load doubleword from memory. */
> (gdb) bt
> #0 __strlen_power8 () at ../sysdeps/powerpc/powerpc64/power8/strlen.S:47
> #1 0x00007ffff7de3d10 in __GI__IO_fputs (str=<optimized out>,
> fp=<optimized out>) at iofputs.c:33
> #2 0x000000001000c7a0 in write_propval (prop=0x100525e0,
> f=0x7ffff7f718a0 <_IO_2_1_stdout_>) at treesource.c:245
>
> The offending line is:
>
> fprintf(f, "%s", delim_start[emit_type]);
>
> where emit_type is TYPE_BLOB and:
>
> static const char *delim_start[] = {
> [TYPE_UINT8] = "[",
> [TYPE_UINT16] = "/bits/ 16 <",
> [TYPE_UINT32] = "<",
> [TYPE_UINT64] = "/bits/ 64 <",
> [TYPE_STRING] = "",
> };
>
> /* Data blobs */
> enum markertype {
> TYPE_NONE,
> REF_PHANDLE,
> REF_PATH,
> LABEL,
> TYPE_UINT8,
> TYPE_UINT16,
> TYPE_UINT32,
> TYPE_UINT64,
> TYPE_BLOB,
> TYPE_STRING,
> };
>
> Because TYPE_BLOB < TYPE_STRING and delim_start[] is a static array,
> delim_start[emit_type] is 0x0. The glibc usually prints out "(null)"
> when one passes 0x0 to %s, but it seems to call fputs() internally if
> the format is exactly "%s", hence the crash.
>
> TYPE_BLOB basically means the data comes from a file and we don't know
> its type. We don't care for the former, and the latter is TYPE_NONE.
>
> So let's drop TYPE_BLOB completely and use TYPE_NONE instead when reading
> the file. Then, try to guess the data type at emission time, like the
> code already does for refs and labels.
>
> Instead of adding yet another check for TYPE_NONE, an helper is introduced
> to check if the data marker has type information, ie, >= TYPE_UINT8.
>
> Fixes: 32b9c61307629ac76c6ac0bead6f926d579b3d2c
> Suggested-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Greg Kurz <groug@xxxxxxxx>
Nice, applied.
Adding a testcase that would catch the explosion in -I fs would be
even better..
> ---
> data.c | 2 +-
> dtc.h | 1 -
> treesource.c | 9 +++++++--
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/data.c b/data.c
> index accdfaef6668..4a204145cc7b 100644
> --- a/data.c
> +++ b/data.c
> @@ -95,7 +95,7 @@ struct data data_copy_file(FILE *f, size_t maxlen)
> {
> struct data d = empty_data;
>
> - d = data_add_marker(d, TYPE_BLOB, NULL);
> + d = data_add_marker(d, TYPE_NONE, NULL);
> while (!feof(f) && (d.len < maxlen)) {
> size_t chunksize, ret;
>
> diff --git a/dtc.h b/dtc.h
> index 303c2a6a73b7..51c03ef64dbe 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -82,7 +82,6 @@ enum markertype {
> TYPE_UINT16,
> TYPE_UINT32,
> TYPE_UINT64,
> - TYPE_BLOB,
> TYPE_STRING,
> };
> extern const char *markername(enum markertype markertype);
> diff --git a/treesource.c b/treesource.c
> index f99544d72344..53e62036ad0e 100644
> --- a/treesource.c
> +++ b/treesource.c
> @@ -133,9 +133,14 @@ static void write_propval_int(FILE *f, const char *p, size_t len, size_t width)
> }
> }
>
> +static bool has_data_type_information(struct marker *m)
> +{
> + return m->type >= TYPE_UINT8;
> +}
> +
> static struct marker *next_type_marker(struct marker *m)
> {
> - while (m && (m->type == LABEL || m->type == REF_PHANDLE || m->type == REF_PATH))
> + while (m && !has_data_type_information(m))
> m = m->next;
> return m;
> }
> @@ -225,7 +230,7 @@ static void write_propval(FILE *f, struct property *prop)
> size_t chunk_len;
> const char *p = &prop->val.val[m->offset];
>
> - if (m->type < TYPE_UINT8)
> + if (!has_data_type_information(m))
> continue;
>
> chunk_len = type_marker_length(m);
>
--
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
Attachment:
signature.asc
Description: PGP signature
