Re: [Qemu-devel] [PULL 2/4] coverity: Model GLib string allocation partially

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> On 05/02/2015 17:24, Markus Armbruster wrote:
>> +
>> +char *g_strdup(const char *s)
>> +{
>> +    char *dup;
>> +    size_t i;
>> +
>> +    if (!s) {
>> +        return NULL;
>> +    }
>> +
>> +    __coverity_string_null_sink__(s);
>> +    __coverity_string_size_sink__(s);
>
> What's __coverity_string_size_sink__?  It is likely responsible for this
> in libcacard:
>
> Unbounded source buffer (STRING_SIZE)
> string_size: Passing string argv[argc - 2] of unknown size to g_strdup,
> which expects a string of a particular size

Yup, it is.  The reference manual explains it "indicates to the
STRING_SIZE checker that a function is a string size sink and must be
protected from arbitrarily large strings."

Of course, treating argv[] strings as unbounded in size is debatable.
The OS generally imposes some limit.  Linux's limit is rather generous:
32 pages, if I read execve(2) correctly.

That aside, why on earth are we copying these strings?  The only use of
the copies is passing them to getaddrinfo() via connect_to_qemu().

> I guess it's okay to mark this as intentional?

I guess it's okay to delete the copying.

>> +char *g_strndup(const char *s, size_t n)
>> +{
>> +    char *dup;
>> +    size_t i;
>> +
>> +    __coverity_negative_sink__(n);
>> +
>> +    if (!s) {
>> +        return NULL;
>> +    }
>> +
>> +    dup = g_malloc(n + 1);
        for (i = 0; i < n && (dup[i] = s[i]); i++) ;
        dup[i] = 0;
        return dup;
    }
>
>
> This should be g_malloc0 I think.

g_malloc0() is more faithful to what the function does, but I decided to
use g_malloc() anyway, because I recommend not to rely on the padding
behavior[*].

I think g_strndup() is fine when you want to duplicate a substring.  No
padding then.  I figure this is the common use case.

When you want to duplicate a string, but limit the size of the
duplicate, then it doesn't do the right thing when the string is shorter
than the limit.  Use g_strdup_printf() instead.

What it does is right for something else: allocating a buffer with a
specific size and completely initializing it from a string.  I can't
remember having had a use for that myself.


[*] Looks like the designers of g_strndup() couldn't resist the
temptation to "improve" upon strndup().  Fine, but call it something
else then.