[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict |
Date: |
Tue, 08 Mar 2016 14:50:43 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Peter Xu <address@hidden> writes:
> On Tue, Mar 08, 2016 at 09:12:45AM +0100, Markus Armbruster wrote:
>> Peter Xu <address@hidden> writes:
>> > 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', ' ');;
>
> Yes, I can do that too. Do you think I should split the patchset
> into several smaller ones possibly? So that I can use a 2/2 for this
> specific function, one for the printf() issue, one for the stack
> allocation issue.
Since the format string cleanup and the unbounded stack issue aren't
related, separate patches make sense.
>>
>> > - 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.
>
> Yes. Actually I doubted whether I should do this change... since I
> am not experienced enough to estimate a value which will be 100%
> working all the time. Here I just assumed 256 is big enough for
> qdict keys, which indeed is lack of proof.
>
>>
>> If this was a good idea, I'd recommend to avoid the awkward macro:
>>
>> char key[256];
>> int i;
>>
>> assert(strlen(entry->key) + 1 <= ARRAY_SIZE(key));
>
> Yes, possibly! I forgot ARRAY_SIZE() macro. Thanks to point out.
>
>>
>> There are several other ways to limit the stack usage:
>>
>> 1. Move the array from stack to heap. Fine unless it's on a hot path.
>> As far as I can tell, this dumping business is for HMP and qemu-io,
>> i.e. not hot.
>>
>> 2. Use a stack array for short strings, switch to the heap for large
>> strings. More complex, but may be worth it on a hot path where most
>> strings are short.
>>
>> 3. Instead of creating a new string with '-' replaced for printing,
>> print characters. Can be okay with buffered I/O, but obviously not
>> on a hot path.
>>
>> 4. Like 3., but print substrings not containing '-'.
>
> Thanks for all the suggestions and ideas.
>
> To avoid the limitation of 256 (and also consider this path is not
> hot path), I'd like to choose (1) if you would not mind, in a split
> patch maybe.
Sounds good to me.