On 07/30/2014 09:37 AM, Adrian Sendroiu wrote:
>> -----Original Message-----
>> From: Pedro Alves [mailto:palves@redhat.com]
>> Sent: Tuesday, July 29, 2014 5:55 PM
>> To: Sendroiu Adrian-B46904; gdb-patches@sourceware.org;
>> tromey@redhat.com
>> Subject: Re: [PATCH v2 2/2] mi-out: Implement mi redirection using a
>> stack.
>>
>> I'm guessing we can trigger this by using "save breakpoints" while
>> logging is enabled, like gdb.base/ui-redirect.exp ?
>> I think it'd be very good if a test to the testsuite was added.
>> Sounds like gdb.mi/mi-logging.exp would be a good place?
>>
>> --
>> Thanks,
>> Pedro Alves
>
> This won't trigger the bug, because the logging code doesn't call ui_out_redirect if the interpreter is MI. The way I caught it was through some python script that executes commands and catches their output into a string. For example, if you have
>
> gdb.execute("break main", False, True)
>
> The call sequence will be something like:
> execute_command_to_string
> ui_out_redirect
> execute_command
> ...
> mi_breakpoint_created
> ui_out_redirect
>
> Then, after executing this, the mi_uiout->data->buffer will incorrectly point to a freed ui_file structure, and any subsequent command will overwrite the pointers inside this ui_file with random data, causing a crash.
>
> Do you have any suggestions on how to make a test case from this scenario?
I'm not sure what specific suggestion you're looking after. :-)
Sound like you'd add a test that does that exactly ?
You'd either base on, or add to mi-logging.exp, and do something like:
mi_gdb_test "python gdb.execute("break main", False, True)" ...
mi_gdb_test <some MI command that causes a crash> ...
(and skip the test if skip_python_tests is true)
Thanks,
Pedro Alves
@@ -22,14 +22,17 @@
#include "defs.h"
#include "ui-out.h"
#include "mi-out.h"
+#include "vec.h"
+
+typedef struct ui_file *ui_filep;
+DEF_VEC_P (ui_filep);
struct ui_out_data
{
int suppress_field_separator;
int suppress_output;
int mi_version;
- struct ui_file *buffer;
- struct ui_file *original_buffer;
+ VEC (ui_filep) *streams;
};
typedef struct ui_out_data mi_out_data;
@@ -215,17 +218,20 @@ mi_field_string (struct ui_out *uiout, int fldno, int width,
enum ui_align align, const char *fldname, const char *string)
{
mi_out_data *data = ui_out_data (uiout);
+ struct ui_file *stream;
if (data->suppress_output)
return;
+ stream = VEC_last (ui_filep, data->streams);
+
field_separator (uiout);
if (fldname)
- fprintf_unfiltered (data->buffer, "%s=", fldname);
- fprintf_unfiltered (data->buffer, "\"");
+ fprintf_unfiltered (stream, "%s=", fldname);
+ fprintf_unfiltered (stream, "\"");
if (string)
- fputstr_unfiltered (string, '"', data->buffer);
- fprintf_unfiltered (data->buffer, "\"");
+ fputstr_unfiltered (string, '"', stream);
+ fprintf_unfiltered (stream, "\"");
}
/* This is the only field function that does not align. */
@@ -236,17 +242,20 @@ mi_field_fmt (struct ui_out *uiout, int fldno, int width,
const char *format, va_list args)
{
mi_out_data *data = ui_out_data (uiout);
+ struct ui_file *stream;
if (data->suppress_output)
return;
+ stream = VEC_last (ui_filep, data->streams);
+
field_separator (uiout);
if (fldname)
- fprintf_unfiltered (data->buffer, "%s=\"", fldname);
+ fprintf_unfiltered (stream, "%s=\"", fldname);
else
- fputs_unfiltered ("\"", data->buffer);
- vfprintf_unfiltered (data->buffer, format, args);
- fputs_unfiltered ("\"", data->buffer);
+ fputs_unfiltered ("\"", stream);
+ vfprintf_unfiltered (stream, format, args);
+ fputs_unfiltered ("\"", stream);
}
void
@@ -275,8 +284,9 @@ void
mi_flush (struct ui_out *uiout)
{
mi_out_data *data = ui_out_data (uiout);
+ struct ui_file *stream = VEC_last (ui_filep, data->streams);
- gdb_flush (data->buffer);
+ gdb_flush (stream);
}
int
@@ -285,15 +295,9 @@ mi_redirect (struct ui_out *uiout, struct ui_file *outstream)
mi_out_data *data = ui_out_data (uiout);
if (outstream != NULL)
- {
- data->original_buffer = data->buffer;
- data->buffer = outstream;
- }
- else if (data->original_buffer != NULL)
- {
- data->buffer = data->original_buffer;
- data->original_buffer = NULL;
- }
+ VEC_safe_push (ui_filep, data->streams, outstream);
+ else
+ VEC_pop (ui_filep, data->streams);
return 0;
}
@@ -306,29 +310,31 @@ static void
field_separator (struct ui_out *uiout)
{
mi_out_data *data = ui_out_data (uiout);
+ struct ui_file *stream = VEC_last (ui_filep, data->streams);
if (data->suppress_field_separator)
data->suppress_field_separator = 0;
else
- fputc_unfiltered (',', data->buffer);
+ fputc_unfiltered (',', stream);
}
static void
mi_open (struct ui_out *uiout, const char *name, enum ui_out_type type)
{
mi_out_data *data = ui_out_data (uiout);
+ struct ui_file *stream = VEC_last (ui_filep, data->streams);
field_separator (uiout);
data->suppress_field_separator = 1;
if (name)
- fprintf_unfiltered (data->buffer, "%s=", name);
+ fprintf_unfiltered (stream, "%s=", name);
switch (type)
{
case ui_out_type_tuple:
- fputc_unfiltered ('{', data->buffer);
+ fputc_unfiltered ('{', stream);
break;
case ui_out_type_list:
- fputc_unfiltered ('[', data->buffer);
+ fputc_unfiltered ('[', stream);
break;
default:
internal_error (__FILE__, __LINE__, _("bad switch"));
@@ -339,14 +345,15 @@ static void
mi_close (struct ui_out *uiout, enum ui_out_type type)
{
mi_out_data *data = ui_out_data (uiout);
+ struct ui_file *stream = VEC_last (ui_filep, data->streams);
switch (type)
{
case ui_out_type_tuple:
- fputc_unfiltered ('}', data->buffer);
+ fputc_unfiltered ('}', stream);
break;
case ui_out_type_list:
- fputc_unfiltered (']', data->buffer);
+ fputc_unfiltered (']', stream);
break;
default:
internal_error (__FILE__, __LINE__, _("bad switch"));
@@ -360,8 +367,9 @@ void
mi_out_buffered (struct ui_out *uiout, char *string)
{
mi_out_data *data = ui_out_data (uiout);
+ struct ui_file *stream = VEC_last (ui_filep, data->streams);
- fprintf_unfiltered (data->buffer, "%s", string);
+ fprintf_unfiltered (stream, "%s", string);
}
/* Clear the buffer. */
@@ -370,8 +378,9 @@ void
mi_out_rewind (struct ui_out *uiout)
{
mi_out_data *data = ui_out_data (uiout);
+ struct ui_file *stream = VEC_last (ui_filep, data->streams);
- ui_file_rewind (data->buffer);
+ ui_file_rewind (stream);
}
/* Dump the buffer onto the specified stream. */
@@ -386,9 +395,10 @@ void
mi_out_put (struct ui_out *uiout, struct ui_file *stream)
{
mi_out_data *data = ui_out_data (uiout);
+ struct ui_file *current_stream = VEC_last (ui_filep, data->streams);
- ui_file_put (data->buffer, do_write, stream);
- ui_file_rewind (data->buffer);
+ ui_file_put (current_stream, do_write, stream);
+ ui_file_rewind (current_stream);
}
/* Return the current MI version. */
@@ -407,13 +417,14 @@ struct ui_out *
mi_out_new (int mi_version)
{
int flags = 0;
+ struct ui_file *new_stream;
mi_out_data *data = XNEW (mi_out_data);
data->suppress_field_separator = 0;
data->suppress_output = 0;
data->mi_version = mi_version;
- /* FIXME: This code should be using a ``string_file'' and not the
- TUI buffer hack. */
- data->buffer = mem_fileopen ();
+ data->streams = NULL;
+ new_stream = mem_fileopen ();
+ VEC_safe_push (ui_filep, data->streams, new_stream);
return ui_out_new (&mi_ui_out_impl, data, flags);
}