Kevin Wolf <kw...@redhat.com> writes: > Am 09.03.2016 um 14:23 hat Markus Armbruster geschrieben: >> Kevin Wolf <kw...@redhat.com> writes: >> >> > Am 09.03.2016 um 04:04 hat Eric Blake geschrieben: >> >> On 03/08/2016 07:57 PM, Peter Xu wrote: >> >> > On Tue, Mar 08, 2016 at 11:19:44AM +0100, Kevin Wolf wrote: >> >> >> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben: >> >> >>> Same arguments as for PATCH 2, except here an argument on the maximum >> >> >>> length of subqdict would probably be easier. >> >> >> >> >> >> Yes, these are constant string literals in all callers, including the >> >> >> one non-test case in quorum. >> >> >> >> >> >> Let's simply assert a reasonable maximum for subqdict_length. The >> >> >> minimum we need to allow with the existing callers is 9, and I expect >> >> >> we'll never get keys longer than 16 characters. >> >> > >> >> > Hi, Kevin, Markus, >> >> > >> >> > The patch should be trying to do as mentioned above. To make it >> >> > clearer, how about the following one: >> >> > >> >> > diff --git a/qobject/qdict.c b/qobject/qdict.c >> >> > index 9833bd0..dde99e0 100644 >> >> > --- a/qobject/qdict.c >> >> > +++ b/qobject/qdict.c >> >> > @@ -704,17 +704,16 @@ int qdict_array_entries(QDict *src, const char >> >> > *subqdict) >> >> > for (i = 0; i < INT_MAX; i++) { >> >> > QObject *subqobj; >> >> > int subqdict_entries; >> >> > - size_t slen = 32 + subqdict_len; >> >> > - char indexstr[slen], prefix[slen]; >> >> > + char indexstr[128], prefix[128]; >> >> > size_t snprintf_ret; >> >> > >> >> > - snprintf_ret = snprintf(indexstr, slen, "%s%u", subqdict, i); >> >> > - assert(snprintf_ret < slen); >> >> > + snprintf_ret = snprintf(indexstr, ARRAY_SIZE(indexstr), >> >> > "%s%u", subqdict, i); >> >> > + assert(snprintf_ret < ARRAY_SIZE(indexstr)); >> >> >> >> sizeof(indexstr) works, and is a bit nicer than ARRAY_SIZE() when >> >> dealing with char. >> >> >> >> But I'm worried that this can trigger an abort() by someone hammering on >> >> the command line. Just because we don't expect any QMP command to >> >> validate with a key name longer than 128 doesn't mean that we don't have >> >> to deal with a command line with a garbage key name that long. What's >> >> wrong with just using g_strdup_printf() and heap-allocating the result, >> >> avoiding snprintf() and fixed lengths altogether? >> > >> > I can only repeat myself, we're not dealing with user data here, but >> > with constant literal strings. Put an assert(subqdict_len < 32); at the >> > beginning of the function and be done with it. Any violation of it is >> > not unexpected user input, but a caller bug. >> >> The fact that the keys are literals is a non-local, not-quite-obvious >> argument. >> >> It's non-local, because in qdict.c we don't know what its users store in >> their qdicts. >> >> It's not quite obvious for the only user so far, quorum_open(). New >> uses outside the block layer are possible, but seem unlikely; the block >> layer is the most demanding user of QDicts. >> >> The block layer's use of qdicts and QemuOpts is "interesting" enough for >> me to call it not quite obvious. In particular, QemuOpts can either >> accept a fixed set of keys, or arbitrary keys. In the latter case, >> unknown keys should be rejected by the consumer of the QemuOpts. >> Whether that happens before they can reach qdict_array_entries() is not >> obvious to me. > > And it doesn't matter here because the variable part that determines the > array size isn't the length of a key in the QDict, but the key name > prefix we're looking for, i.e. the name of the QAPI array we want to > parse. Unless you plan to introduce computed field names in QAPI, I > can't imagine a reason why this could ever be something else than a > constant string.
I think I see now. >> We can rely on non-local or subtle arguments when it's worthwhile, but >> I'm not sure it's worthwhile here. I'd use g_strdup_printf() and call >> it a day. > > I think it's unnecessary, but fine with me. I'm just trying to say that > making it a fixed 128 byte array on the stack certainly doesn't improve > anything. It trades a few bytes of stack for a fixed stack frame. A fixed stack frame is a bit more efficient (not that it matters here), and lets us scratch the function permanently from the list of stack fram size false positives. I think that's a reasonable trade. [...]