On Thu, 30 Aug 2018 16:16:31 +1000
David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, Aug 24, 2018 at 05:57:33PM +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.
> >
> > This could be fixed by adding TYPE_BLOB as well to delim_start[] and
> > delim_end[], but then we would face an annoying situation where all
> > properties would be printed as a sequence of bytes.
> >
> > So, instead, when a TYPE_BLOB marker is present, let's try to guess the
> > value type and update the marker accordingly. This ensures that emit_type
> > points to a valid entry in delim_start[] and delim_end[], thus fixing the
> > crash. And this gives a chance to choose a prettier format than a sequence
> > of bytes when possible.
> >
> > While here, let's add an assertion to make it clear which types are
> > allowed.
>
> Hrm.. so.. i see the problem.
>
> But is TYPE_BLOB actually worth saving? It basically just says the
> data was read in from a file which tells us.. nothing, really.
>
I agree TYPE_BLOB doesn't seem to be very useful.
> Seems to me data_copy_file() should just set TYPE_NONE, since it can't
> know the type of the data within.
>
Ok, I'll try that. Thanks!
> >
> > Fixes: 32b9c61307629ac76c6ac0bead6f926d579b3d2c
> > Signed-off-by: Greg Kurz <groug@xxxxxxxx>
> > ---
> > treesource.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/treesource.c b/treesource.c
> > index f99544d72344..daf88ed3ce2a 100644
> > --- a/treesource.c
> > +++ b/treesource.c
> > @@ -228,6 +228,16 @@ static void write_propval(FILE *f, struct property *prop)
> > if (m->type < TYPE_UINT8)
> > continue;
> >
> > + if (m->type == TYPE_BLOB)
> > + m->type = guess_value_type(prop);
> > +
> > + /* emit_type MUST have a valid entry in the delimiter arrays */
> > + assert(m->type == TYPE_UINT8 ||
> > + m->type == TYPE_UINT16 ||
> > + m->type == TYPE_UINT32 ||
> > + m->type == TYPE_UINT64 ||
> > + m->type == TYPE_STRING);
> > +
> > chunk_len = type_marker_length(m);
> > if (!chunk_len)
> > chunk_len = len - m->offset;
> >
>
Attachment:
pgpJugc1edA09.pgp
Description: OpenPGP digital signature
