Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing

[Tom Tromey <tom@tromey.com>]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> There was also a second regression which was fixed by the patch
Philippe> suggested in the RFA 1/2 (and the test in RFA 2/2 was checking
Philippe> the regression).
Philippe> Will you add the fix for the second regression (and the test)
Philippe> in another commit ?

Yeah, I can rebase your patches on top of this one.

>> if (repeat && *p1 == '\0')
>> -    return saved_command_line;
>> +    {
>> +      xfree (buffer_finish (cmd_line_buffer));
>> +      buffer_grow_str0 (cmd_line_buffer, saved_command_line);
>> +      cmd_line_buffer->used_size = 0;

Philippe> I was somewhat surprised by used_size being set to 0.
Philippe> I am not sure to understand in which case it is supposed to be
Philippe> set to 0 : it is set to 0 in the first few lines of handle_line_of_input
Philippe> with a comment explaining why.
Philippe> I however do not understand why it is set to the string length in
Philippe> the 'Do history expansion' case, and not to 0 : as far as I can see,
Philippe> cmd will be returned as a full line in case of expansion ?

Thanks for noticing this.

I suspect the history case (which doesn't seem to be tested...) is just
wrong and should also set the used size to 0.

I believe the idea being clear the used size is that the buffer still
owns the string, but if the buffer is reused then the string is
considered ok to overwrite.  Which, admittedly, seems odd to me, but
this patch seemed like the least intrusive way to avoid the
use-after-free...

I'll see if I can come up with a test case here.

Philippe> IIUC, the returned value will stay valid as long as the
Philippe> cmd_line_buffer is not destroyed but also only if the buffer is
Philippe> not touched (e.g. no data can be added to it, as otherwise
Philippe> the previously returned value might become dangling).
Philippe> So, for example, the below (pseudo) code would be incorrect:
Philippe>    arg = command_line_input (..., cmd_buffer);
Philippe>    while parsing arg not finished
Philippe>        // here it is forbidden to touch cmd_buffer
Philippe>        // e.g. the below would be bad:
Philippe>        some_other_arg = command_line_input (..., cmd_buffer);

Philippe> So, I am wondering what kind of usage of this function
Philippe> will make the buffer bigger. Maybe worth pointing at the
Philippe> above risk then ?

I can add a comment somewhere.

I don't really understand why this code works the way it does.
It seems unusually convoluted.  At the same time, I didn't want to
completely tear it apart, since it's not clear that's a good use of
one's time.

Tom