[PATCH] gdb: remove local extern declaration of cli_styling

[PATCH] gdb: remove local extern declaration of cli_styling

[Simon Marchi]

Following the int -> bool conversion of boolean options (commit
491144b5e21b ("Change boolean options to bool instead of int")), I see
this ASAN error:

  ==357543==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55555b25d440 at pc 0x5555583ce9e1 bp 0x7fffffffd390 sp 0x7fffffffd380
  READ of size 4 at 0x55555b25d440 thread T0
      #0 0x5555583ce9e0 in term_cli_styling /home/simark/src/binutils-gdb/gdb/ui-file.c:111
      #1 0x5555583cf8b0 in stdio_file::can_emit_style_escape() /home/simark/src/binutils-gdb/gdb/ui-file.c:308
      #2 0x5555584450d2 in set_output_style /home/simark/src/binutils-gdb/gdb/utils.c:1442
      #3 0x5555584491af in fprintf_styled(ui_file*, ui_file_style const&, char const*, ...) /home/simark/src/binutils-gdb/gdb/utils.c:2143
      #4 0x5555582fa13c in print_gdb_version(ui_file*, bool) /home/simark/src/binutils-gdb/gdb/top.c:1339
      #5 0x555557b723ab in captured_main_1 /home/simark/src/binutils-gdb/gdb/main.c:981
      #6 0x555557b7353b in captured_main /home/simark/src/binutils-gdb/gdb/main.c:1172
      #7 0x555557b735d0 in gdb_main(captured_main_args*) /home/simark/src/binutils-gdb/gdb/main.c:1197
      #8 0x55555700a53d in main /home/simark/src/binutils-gdb/gdb/gdb.c:32
      #9 0x7ffff64c9ee2 in __libc_start_main (/usr/lib/libc.so.6+0x26ee2)
      #10 0x55555700a30d in _start (/home/simark/build/binutils-gdb/gdb/gdb+0x1ab630d)

  0x55555b25d441 is located 0 bytes to the right of global variable 'cli_styling' defined in '/home/simark/src/binutils-gdb/gdb/cli/cli-style.c:31:6' (0x55555b25d440) of size 1

The reason of this is that we have this declaration of cli_styling in cli/cli-style.h:

  extern bool cli_styling;

but ui-file.c uses its  own local declaration:

  extern int cli_styling;

Because of that, the code in ui-file.c thinks the variable is 4 bytes
long, when it is actually 1 byte long, so the generated code reads past
the variable.

Fix it by removing the declaration and making ui-file.c include
cli/cli-style.h.

gdb/ChangeLog:

	* ui-file.c: Include cli/cli-style.h.
	(term_cli_styling): Remove cli_styling declaration.
---
 gdb/ui-file.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index 042b13ca3b18..71b74bba1905 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -24,6 +24,7 @@
 #include "gdb_obstack.h"
 #include "gdb_select.h"
 #include "gdbsupport/filestuff.h"
+#include "cli/cli-style.h"
 
 null_file null_stream;
 
@@ -106,8 +107,6 @@ ui_file_isatty (struct ui_file *file)
 static bool
 term_cli_styling ()
 {
-  extern int cli_styling;
-
   if (!cli_styling)
     return false;
 
-- 
2.23.0

Re: [PATCH] gdb: remove local extern declaration of cli_styling

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> 	* ui-file.c: Include cli/cli-style.h.
Simon> 	(term_cli_styling): Remove cli_styling declaration.

Thanks for doing this.

I think as a rule it's best to always have declarations in a header and
never in the .c code.  That way they can be checked against the
definition.

Tom

Re: [PATCH] gdb: remove local extern declaration of cli_styling

[Simon Marchi]

On 2019-09-18 12:18 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> 	* ui-file.c: Include cli/cli-style.h.
> Simon> 	(term_cli_styling): Remove cli_styling declaration.
> 
> Thanks for doing this.
> 
> I think as a rule it's best to always have declarations in a header and
> never in the .c code.  That way they can be checked against the
> definition.

Yes, agreed.

Actually, I didn't push this patch directly, even though it seemed obvious, because
I was wondering if there was some good reason I was missing for not doing it like
this in the first place.

I pushed it now.

Simon