> On 9 Feb 2018, at 17:59, Frediano Ziglio <fzig...@redhat.com> wrote:
>
>>>>
>>>>
>>>>> +
>>>>> + // read part of the message till we get all
>>>>> + if (dev->msg_len < dev->hdr.size) {
>>>>> + dev->msg = g_realloc(dev->msg, dev->hdr.size);
>>>>> + dev->msg_len = dev->hdr.size;
>>>>> + }
>>>>> + int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size -
>>>>> dev->msg_pos);
>>>>> + if (n <= 0) {
>>>>> + return false;
>>>>> + }
>>>>> + dev->msg_pos += n;
>>>>> + if (dev->msg_pos != dev->hdr.size) {
>>>>
>>>> Is it assumed you can all read in one go? I’m surprized there is a “while”
>>>> loop for reading the header (which is small) but no while loop for reading
>>>> the payload which is larger).
>>>>
>>>
>>> When we return false is not an error, just there's no data left next time
>>> will add more data. So there's no while but there's a loop anyway.
>>> I suppose similarly we could avoid the while in the other function.
>>
>> Oh, so you are saying that you think it’s OK to loop ouside, truncate the
>> data that you g_realloc’d, and then start over?
>>
>> Does not work. Say you started with 1K in buffer, then go to this inner read
>> here, which does a g_realloc and reads 10K. Then we re-enter the above
>> function, so I believe we will truncate to 1K again, and then re-extend to
>> 10K and start reading at 10K. We lost 9K of data, unless I’m mistaken.
>>
>
> No, the header is not read again until all message is processed.
> There are also tests for this.
This is not what I am talking about. I’m saying you read this, where H is
header and D is data.
You first read header, let’s say four items.
[HHHH]
Then you realloc, which puts some undefined garbage in the area the data:
[HHHH][????????????]
Then you read data, get 4 elements:
[HHHH][DDDD????????]
So you restart in the outer loop, where you truncate the header
[HHHH]
and reallocate
[HHHH][????????????]
and restart reading, not sure if we have reset msg_pos or not, so you get either
[HHHH][DDDDDDDD????]
or
[HHHH][????DDDDDDDD]
None of which is correct. I’ve not seen in the code how we prevent this if we
don’t loop in the inner read.
Maybe I missed the part that makes us restart correctly, but where did we keep
the data?
Regards
Christophe
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
