[PULL,2/4] coverity: Model GLib string allocation partially

Without a model, Coverity can't know that the result of g_strdup()
needs to be fed to g_free().

One way to get such a model is to scan GLib, build a derived model
file with cov-collect-models, and use that when scanning QEMU.
Unfortunately, the Coverity Scan service we use doesn't support that.

Thus, we're stuck with the other way: write a user model.  Doing that
for all of GLib is hardly practical.  I'm doing it for the "String
Utility Functions" we actually use that return dynamically allocated
strings.

In a local scan, this flags 20 additional RESOURCE_LEAKs.  The ones I
checked look genuine.

It also loses a NULL_RETURNS about ppce500_init() using
qemu_find_file() without error checking.  I don't understand why.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-model.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

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

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

> 
> +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);


This should be g_malloc0 I think.

Paolo

Paolo Bonzini <pbonzini@redhat.com> 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.