diff mbox

Fix UB in diagnostic.c

Message ID 20140814172205.GC28810@redhat.com
State New
Headers show

Commit Message

Marek Polacek Aug. 14, 2014, 5:22 p.m. UTC 
On Wed, Aug 13, 2014 at 09:03:37PM +0200, Manuel López-Ibáñez wrote:
> I don't think this is the right fix. The problem is that we are trying
> to print the caret in a column that is larger than the line_width. We
> do this because the file given by the line directive has nothing to do
> with the actual code we are parsing. I think in that case it is ok to
> just give up and not print a caret. So something like:
> 
> @@ -300,11 +299,11 @@ diagnostic_show_locus (diagnostic_contex
>      return;
> 
>    context->last_location = diagnostic->location;
>    s = expand_location_to_spelling_point (diagnostic->location);
>    line = location_get_source_line (s, &line_width);
> -  if (line == NULL)
> +  if (line == NULL || s.column > line_width)
>      return;
> 
>    max_width = context->caret_max_width;
>    line = adjust_line (line, line_width, max_width, &(s.column));
> 
> Nonetheless, perhaps in addition adjust_line should have
> gcc_checking_assert(line_width >= column).

Yea, I think this would be much better, thanks.  Done in the
following.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2014-08-14  Marek Polacek  <polacek@redhat.com>
	    Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR c/62059
	* diagnostic.c (adjust_line): Add gcc_checking_assert.
	(diagnostic_show_locus): Don't print caret diagnostic
	if a column is larger than the line_width.


	Marek

Comments

Dodji Seketeli Aug. 17, 2014, 10:37 a.m. UTC | #1 
Marek Polacek <polacek@redhat.com> a écrit:

> On Wed, Aug 13, 2014 at 09:03:37PM +0200, Manuel López-Ibáñez wrote:
>> I don't think this is the right fix. The problem is that we are trying
>> to print the caret in a column that is larger than the line_width. We
>> do this because the file given by the line directive has nothing to do
>> with the actual code we are parsing. I think in that case it is ok to
>> just give up and not print a caret. So something like:
>> 
>> @@ -300,11 +299,11 @@ diagnostic_show_locus (diagnostic_contex
>>      return;
>> 
>>    context->last_location = diagnostic->location;
>>    s = expand_location_to_spelling_point (diagnostic->location);
>>    line = location_get_source_line (s, &line_width);
>> -  if (line == NULL)
>> +  if (line == NULL || s.column > line_width)
>>      return;
>> 
>>    max_width = context->caret_max_width;
>>    line = adjust_line (line, line_width, max_width, &(s.column));
>> 
>> Nonetheless, perhaps in addition adjust_line should have
>> gcc_checking_assert(line_width >= column).
>
> Yea, I think this would be much better, thanks.  Done in the
> following.

Yes, I agree with this as well.

> Bootstrapped/regtested on x86_64-linux, ok for trunk?

Yes, this is OK to me.

Thank you for looking into this.

Cheers.
diff mbox

Patch

diff --git gcc/diagnostic.c gcc/diagnostic.c
index 0cc7593..2109f31 100644
--- gcc/diagnostic.c
+++ gcc/diagnostic.c
@@ -270,6 +270,7 @@  adjust_line (const char *line, int line_width,
   int right_margin = 10;
   int column = *column_p;
 
+  gcc_checking_assert (line_width >= column);
   right_margin = MIN (line_width - column, right_margin);
   right_margin = max_width - right_margin;
   if (line_width >= max_width && column > right_margin)
@@ -302,7 +303,7 @@  diagnostic_show_locus (diagnostic_context * context,
   context->last_location = diagnostic->location;
   s = expand_location_to_spelling_point (diagnostic->location);
   line = location_get_source_line (s, &line_width);
-  if (line == NULL)
+  if (line == NULL || s.column > line_width)
     return;
 
   max_width = context->caret_max_width;