On Fri, 31 Aug 2018 11:16:27 +1000
David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> 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..
>
Indeed... but I should learn about dtc testing first. I may try to find
some spare cycles to do that, but not sure this can happen anytime soon.
> > ---
> > 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);
> >
>
Attachment:
pgpQzji9d79so.pgp
Description: OpenPGP digital signature
