[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict |
Date: |
Tue, 08 Mar 2016 14:47:31 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Fam Zheng <address@hidden> writes:
> On Tue, 03/08 09:12, Markus Armbruster wrote:
>> Peter Xu <address@hidden> writes:
>>
>> > Suggested-by: Paolo Bonzini <address@hidden>
>> > CC: Markus Armbruster <address@hidden>
>> > CC: Kevin Wolf <address@hidden>
>> > CC: address@hidden
>> > Signed-off-by: Peter Xu <address@hidden>
>> > ---
>> > block/qapi.c | 5 ++++-
>> > 1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/block/qapi.c b/block/qapi.c
>> > index db2d3fb..687e577 100644
>> > --- a/block/qapi.c
>> > +++ b/block/qapi.c
>> > @@ -638,9 +638,12 @@ static void dump_qdict(fprintf_function func_fprintf,
>> > void *f, int indentation,
>> > QType type = qobject_type(entry->value);
>> > bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
>> > const char *format = composite ? "%*s%s:\n" : "%*s%s: ";
>>
>> Unrelated to your patch: ugh!
>>
>> Printf formats should be literals whenever possible, to make it easy for
>> the compiler to warn you when you screw up. It's trivially possible
>> here! Instead of
>>
>> func_fprintf(f, format, indentation * 4, "", key);
>>
>> do
>>
>> func_fprintf(f, "%*s%s:%c", indentation * 4, "", key,
>> composite ? '\n', ' ');;
>>
>> > - char key[strlen(entry->key) + 1];
>> > +#define __KEY_LEN (256)
>> > + char key[__KEY_LEN];
>> > int i;
>> >
>> > + assert(strlen(entry->key) + 1 <= __KEY_LEN);
>> > +#undef __KEY_LEN
>> > /* replace dashes with spaces in key (variable) names */
>> > for (i = 0; entry->key[i]; i++) {
>> > key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
>>
>> I'm afraid this isn't a good idea. It relies on the non-local argument
>> that nobody will ever put a key longer than 255 into a qdict that gets
>> dumped. That may even be the case, but you need to *prove* it, not just
>> assert it. The weakest acceptable proof might be assertions in every
>> place that put keys into a dict that might get dumped. I suspect that's
>> practical and maintainable only if there's a single place that does it.
>>
>> If this was a good idea, I'd recommend to avoid the awkward macro:
>
> Also I think the double underscore identifiers are considered reserved in C,
> no?
Correct. C99 7.1.3 Reserved identifiers: All identifiers that begin
with an underscore and either an uppercase letter or another underscore
are always reserved for any use.
[...]
[Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict, Peter Xu, 2016/03/08
Re: [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict, Peter Xu, 2016/03/08
Re: [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict, Markus Armbruster, 2016/03/08
Re: [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict, Paolo Bonzini, 2016/03/08
Re: [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict, Peter Xu, 2016/03/08
[Qemu-devel] [PATCH 3/8] usb: fix unbounded stack for ohci_td_pkt, Peter Xu, 2016/03/08
[Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s, Peter Xu, 2016/03/08