[PATCH v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Jinoh Kang]

On 11/19/21 23:54, Rémi Bernon wrote:
> Hi Jinoh!
> 
> A few comments:
> 
> On 11/19/21 14:41, Jinoh Kang wrote:
>> Signed-off-by: Jinoh Kang <jinoh.kang.kr at gmail.com>
>> ---
> 
> Overall I'd say this one is probably worth splitting. Maybe consider a bit of prior refactoring to use the same "reply_buffer" both for the packet output buffer and the qxfer buffer?

De-duplicating the buffer management code sounds like a good idea.

My intention was to minimise touching existing code and do refactoring in
a separate patch serie.  Turns out this extra step is unnecessary, I guess.

> 
> At least it'd be nice to have the escaping done separately from the qxfer buffer addition, if possible.

I agree.  In this case I would say that the qXfer buffer addition logically
precedes the XML escaping.  The reason is that the qXfer buffer is required to
correctly calculate the offset and length of the reply data slice that should
be transmitted to the debugger client.

Currently gdbproxy reuses the same buffer for both qXfer reply and actual GDB
packet reply.  This works well since a character in the qXfer reply buffer
matches 1:1 to the actual GDB reply packet.  However, if special characters are
escaped, this property will no longer hold and a single byte in qXfer reply will
take up to two bytes in the GDB reply packet.  This causes offsets to shift,
preventing the offset/length response slicing from working correctly.

I'll copy the above in the commit message.

> 
>>   +static void reply_buffer_resize(struct reply_buffer* reply, size_t alloc)
>> +{
>> +    reply->alloc = alloc;
>> +    reply->base = realloc(reply->base, reply->alloc);
>> +}
>> +
>> +static void reply_buffer_empty(struct reply_buffer* reply)
>> +{
>> +    reply->len = 0;
>> +    reply_buffer_resize(reply, 0);
>> +}
>> +
> 
> Do we need to realloc when emptying? I think the less alloc call we do the better.

I just noticed that gdbproxy does not free the old packet reply after transmission.
There's no reason reply_buffer shall deviate from this behaviour.  Thanks!

> I don't think we should worry too much about memory consumption here?

In fact, if memory consumption isn't a concern, why don't we straight out just
pre-allocate buffers in fixed sizes?  This way we don't have to worry about
realloc() entirely (we would still need to look out for overflows though).
This is also what some GDB stubs (e.g. the Linux kernel) does.
Perhaps fifteen pages followed by a guard to catch overlooked memory corruption
issues would suffice?

> 
>> +static void reply_buffer_grow(struct reply_buffer* reply, size_t size)
>> +{
>> +    if (reply->alloc < reply->len + size)
>> +        reply_buffer_resize(reply, ((reply->len + size) / 32 + 1) * 32);
>> +}
>> +
> 
> Maybe just one grow helper is enough and we can drop the resize? Otherwise I'd also expect it to work the other way, where resize only realloc when needed, calling grow.

Yes, helper merging will also cover this case.

> 
> [...]
> 
>>   static enum packet_return packet_query(struct gdb_context* gdbctx)
>> @@ -1929,32 +2045,50 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
>>       case 'X':
>>           if (sscanf(gdbctx->in_packet, "Xfer:libraries:read::%x,%x", &off, &len) == 2)
>>           {
>> +            BOOL more;
>> +
>>               if (!gdbctx->process) return packet_error;
>>   -            packet_reply_open_xfer(gdbctx);
>> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>>               packet_query_libraries(gdbctx);
>> -            packet_reply_close_xfer(gdbctx, off, len);
>> +            packet_reply_xfer(gdbctx,
>> +                              gdbctx->qxfer_buffer.base,
>> +                              gdbctx->qxfer_buffer.len,
>> +                              off, len, &more);
>> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>>               return packet_done;
>>           }
>>             if (sscanf(gdbctx->in_packet, "Xfer:threads:read::%x,%x", &off, &len) == 2)
>>           {
>> +            BOOL more;
>> +
>>               if (!gdbctx->process) return packet_error;
>>   -            packet_reply_open_xfer(gdbctx);
>> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>>               packet_query_threads(gdbctx);
>> -            packet_reply_close_xfer(gdbctx, off, len);
>> +            packet_reply_xfer(gdbctx,
>> +                              gdbctx->qxfer_buffer.base,
>> +                              gdbctx->qxfer_buffer.len,
>> +                              off, len, &more);
>> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>>               return packet_done;
>>           }
>>             if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2)
>>           {
>> +            BOOL more;
>> +
> 
> I think these more variables are not even used at this point, you could probably only add them later.

Ack.

> 
>>               if (!gdbctx->process) return packet_error;
>>               if (!(cpu = gdbctx->process->be_cpu)) return packet_error;
>>   -            packet_reply_open_xfer(gdbctx);
>> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>>               packet_query_target_xml(gdbctx, cpu);
>> -            packet_reply_close_xfer(gdbctx, off, len);
>> +            packet_reply_xfer(gdbctx,
>> +                              gdbctx->qxfer_buffer.base,
>> +                              gdbctx->qxfer_buffer.len,
>> +                              off, len, &more);
>> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>>               return packet_done;
>>           }
>>           break;
> 
> Do we really need to empty the buffer before and after?

Since we haven't implemented the cache here, not really.

> 
> Cheers,

-- 
Sincerely,
Jinoh Kang