[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 2/4] coverity: Model GLib string allocation parti
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PULL 2/4] coverity: Model GLib string allocation partially |
Date: |
Thu, 12 Feb 2015 09:52:37 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
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.
- [Qemu-devel] [PULL 0/4] coverity: Improve and extend model, Markus Armbruster, 2015/02/05
- [Qemu-devel] [PULL 4/4] MAINTAINERS: Add myself as Coverity model maintainer, Markus Armbruster, 2015/02/05
- [Qemu-devel] [PULL 1/4] coverity: Improve model for GLib memory allocation, Markus Armbruster, 2015/02/05
- [Qemu-devel] [PULL 3/4] coverity: Model g_free() isn't necessarily free(), Markus Armbruster, 2015/02/05
- [Qemu-devel] [PULL 2/4] coverity: Model GLib string allocation partially, Markus Armbruster, 2015/02/05
- Re: [Qemu-devel] [PULL 0/4] coverity: Improve and extend model, Peter Maydell, 2015/02/05