[Qemu-devel] [trivial for-2.6] util/id: fully allocate names table

[Qemu-devel] [trivial for-2.6] util/id: fully allocate names table

[John Snow]

Trivial: this array should be allocated to have ID_MAX entries always.
Otherwise if someone were to forget to expand this table, the assertion
in the id generator won't actually trigger; it will read junk data.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 util/id.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/id.c b/util/id.c
index bcc64d8..b7ca4d2 100644
--- a/util/id.c
+++ b/util/id.c
@@ -29,7 +29,7 @@ bool id_wellformed(const char *id)
 
 #define ID_SPECIAL_CHAR '#'
 
-static const char *const id_subsys_str[] = {
+static const char *const id_subsys_str[ID_MAX] = {
     [ID_QDEV]  = "qdev",
     [ID_BLOCK] = "block",
 };
-- 
2.4.3

Re: [Qemu-devel] [trivial for-2.6] util/id: fully allocate names table

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Trivial: this array should be allocated to have ID_MAX entries always.
> Otherwise if someone were to forget to expand this table, the assertion
> in the id generator won't actually trigger; it will read junk data.

You mean this one:

    assert(id < ID_MAX);

The assertion is crap, because it fails to protect array access
id_subsys_str[id].  Here's one that does:

    assert(0 <= id && id < ARRAY_SIZE(id_subsys_str));

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  util/id.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/id.c b/util/id.c
> index bcc64d8..b7ca4d2 100644
> --- a/util/id.c
> +++ b/util/id.c
> @@ -29,7 +29,7 @@ bool id_wellformed(const char *id)
>  
>  #define ID_SPECIAL_CHAR '#'
>  
> -static const char *const id_subsys_str[] = {
> +static const char *const id_subsys_str[ID_MAX] = {
>      [ID_QDEV]  = "qdev",
>      [ID_BLOCK] = "block",
>  };

Re: [Qemu-devel] [trivial for-2.6] util/id: fully allocate names table

[Kevin Wolf]

Am 25.11.2015 um 09:18 hat Markus Armbruster geschrieben:
> John Snow <jsnow@redhat.com> writes:
> 
> > Trivial: this array should be allocated to have ID_MAX entries always.
> > Otherwise if someone were to forget to expand this table, the assertion
> > in the id generator won't actually trigger; it will read junk data.
> 
> You mean this one:
> 
>     assert(id < ID_MAX);
> 
> The assertion is crap, because it fails to protect array access
> id_subsys_str[id].  Here's one that does:
> 
>     assert(0 <= id && id < ARRAY_SIZE(id_subsys_str));

Or without the kraxelism id >= 0. However, depending on whether enums
are signed or unsigned, I seem to remember that this could trigger
compiler warnings (comparison is always true). And this one should be
unsigned with gcc because it doesn't include negative values.

Kevin

Re: [Qemu-devel] [trivial for-2.6] util/id: fully allocate names table

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 25.11.2015 um 09:18 hat Markus Armbruster geschrieben:
>> John Snow <jsnow@redhat.com> writes:
>> 
>> > Trivial: this array should be allocated to have ID_MAX entries always.
>> > Otherwise if someone were to forget to expand this table, the assertion
>> > in the id generator won't actually trigger; it will read junk data.
>> 
>> You mean this one:
>> 
>>     assert(id < ID_MAX);
>> 
>> The assertion is crap, because it fails to protect array access
>> id_subsys_str[id].  Here's one that does:
>> 
>>     assert(0 <= id && id < ARRAY_SIZE(id_subsys_str));
>
> Or without the kraxelism id >= 0. However, depending on whether enums
> are signed or unsigned, I seem to remember that this could trigger
> compiler warnings (comparison is always true). And this one should be
> unsigned with gcc because it doesn't include negative values.

Whatever it takes to express the range check in a way the compiler
likes.

Since ARRAY_SIZE()'s value is size_t, and size_t is unsigned, the
comparison with 0 can simply be omitted.

Re: [Qemu-devel] [trivial for-2.6] util/id: fully allocate names table

[John Snow]

On 11/25/2015 03:18 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Trivial: this array should be allocated to have ID_MAX entries always.
>> Otherwise if someone were to forget to expand this table, the assertion
>> in the id generator won't actually trigger; it will read junk data.
> 
> You mean this one:
> 
>     assert(id < ID_MAX);
> 

Well, sort of. I meant 'assert(id_subsys_str[id])' itself. If you forget
to expand the list (It happened to a friend of mine) this assert will
pass because it reads garbage.

If you just always expand the full table, though, it will catch you
(Err, my friend) being a dummy a little more nicely.

My thought is we need both the range and presence checks.
I'll v2 it, thanks.

--js

> The assertion is crap, because it fails to protect array access
> id_subsys_str[id].  Here's one that does:
> 
>     assert(0 <= id && id < ARRAY_SIZE(id_subsys_str));
> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  util/id.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/util/id.c b/util/id.c
>> index bcc64d8..b7ca4d2 100644
>> --- a/util/id.c
>> +++ b/util/id.c
>> @@ -29,7 +29,7 @@ bool id_wellformed(const char *id)
>>  
>>  #define ID_SPECIAL_CHAR '#'
>>  
>> -static const char *const id_subsys_str[] = {
>> +static const char *const id_subsys_str[ID_MAX] = {
>>      [ID_QDEV]  = "qdev",
>>      [ID_BLOCK] = "block",
>>  };