[Spice-devel] [spice-common PATCH 2/2] test-marshallers.proto: ArrayMessage: make space for name

[Uri Lublin]

On 8/11/19 1:02 PM, Frediano Ziglio wrote:
>>
>> Do it by adding @end tag.
>> Without it 'name' is a non-allocated pointer.
>>
>> Signed-off-by: Uri Lublin <uril at redhat.com>
>> ---
>>
>> Is there a better way to do it ?
> 
> Is not clear what you are trying to achieve with this patch.

The problem is currently name is defined as a pointer
but is not allocated.
The generated code tries to memcpy data into name, which is
wrong. I see now the patch is missing a part -- more below.


> 
>>
>> ---
>>
>>   tests/test-marshallers.proto | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/test-marshallers.proto b/tests/test-marshallers.proto
>> index 34cc892..eabd487 100644
>> --- a/tests/test-marshallers.proto
>> +++ b/tests/test-marshallers.proto
>> @@ -6,7 +6,7 @@ channel TestChannel {
>>      } ShortDataSubMarshall;
>>   
>>      message {
>> -      int8 name[];
>> +      int8 name[] @end;
>>      } ArrayMessage;
>>   
>>       message {
> 
> Wondering where this message is used, I cannot find much references
> to it beside autogenerated code.

I think it is not used.

> 
> There's a declaration for the type:
> 
> typedef struct {
>      int8_t *name;
> } SpiceMsgMainArrayMessage;

Yes, this is wrong too -- it needs to be int8_t name[0].

> 
> I tried to use the code to generate the structure definitions and
> I got this instead
> 
> typedef struct SpiceMsgMainArrayMessage {
>      int8_t name[0];
> } SpiceMsgMainArrayMessage;

That's correct. Maybe we should auto-generate this .h file.

> 
> The demarshaller seems to not allocate space for the field which
> seems quite a problem (maybe this is detected by Coverity??).

It is indeed detected by covscan.

> 
> I remember I had a patch with some notes about possible
> security concerned mix of attributes, maybe this was one
> situation.
> There are no occurrences of this mix in spice.proto, either
> you have @end or @as_ptr not arrays with unspecified length.

And this patch is adding @end.

I'll send a V2 -- changing tests/test-marshallers.h too

Thanks,
     Uri.