From 338d6484d186c86d64d6c551fdeeb5ae9df5a316 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Thu, 13 Jul 2017 19:30:42 +0000 Subject: [PATCH] diagnostics: fix crash when consolidating out-of-order fix-it hints (PR c/81405) PR c/81405 identifies a crash when printing fix-it hints from -Wmissing-braces when there are excess elements. The fix-it hints are bogus (which I've filed separately as PR c/81432), but they lead to a crash within the fix-it consolidation logic I added in r247548, in line_corrections::add_hint. The root cause is that some of the fix-it hints are out-of-order with respect to the column numbers they affect, which can lead to negative values when computing the gap between the fix-it hints, leading to bogus memcpy calls that generate out-of-bounds buffer accesses. The fix is to sort the fix-it hints after filtering them, ensuring that the gap >= 0. The patch also adds numerous assertions to the code, both directly, and by moving the memcpy calls and their args behind interfaces (themselves containing gcc_assert). This fixes the crash; it doesn't fix the bug in -Wmissing-braces that leads to the bogus hints. gcc/ChangeLog: PR c/81405 * diagnostic-show-locus.c (fixit_cmp): New function. (layout::layout): Sort m_fixit_hints. (column_range::column_range): Assert that the values are valid. (struct char_span): New struct. (correction::overwrite): New method. (struct source_line): New struct. (line_corrections::add_hint): Add assertions. Reimplement memcpy calls in terms of classes source_line and char_span, and correction::overwrite. (selftest::test_overlapped_fixit_printing_2): New function. (selftest::diagnostic_show_locus_c_tests): Call it. gcc/testsuite/ChangeLog: PR c/81405 * gcc.dg/Wmissing-braces-fixits.c: Add coverage for PR c/81405. */ From-SVN: r250187 --- gcc/ChangeLog | 17 +- gcc/diagnostic-show-locus.c | 193 ++++++++++++++++-- gcc/testsuite/ChangeLog | 7 +- gcc/testsuite/gcc.dg/Wmissing-braces-fixits.c | 25 +++ 4 files changed, 227 insertions(+), 15 deletions(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 2a4f0827e8f..fd5b7bb9512 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,4 +1,19 @@ - 2017-07-13 Will Schmidt +2017-07-13 David Malcolm + + PR c/81405 + * diagnostic-show-locus.c (fixit_cmp): New function. + (layout::layout): Sort m_fixit_hints. + (column_range::column_range): Assert that the values are valid. + (struct char_span): New struct. + (correction::overwrite): New method. + (struct source_line): New struct. + (line_corrections::add_hint): Add assertions. Reimplement memcpy + calls in terms of classes source_line and char_span, and + correction::overwrite. + (selftest::test_overlapped_fixit_printing_2): New function. + (selftest::diagnostic_show_locus_c_tests): Call it. + +2017-07-13 Will Schmidt * config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Return early if there is no lhs. diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c index 5227400b716..b0e72e735bf 100644 --- a/gcc/diagnostic-show-locus.c +++ b/gcc/diagnostic-show-locus.c @@ -756,6 +756,16 @@ compatible_locations_p (location_t loc_a, location_t loc_b) } } +/* Comparator for sorting fix-it hints. */ + +static int +fixit_cmp (const void *p_a, const void *p_b) +{ + const fixit_hint * hint_a = *static_cast (p_a); + const fixit_hint * hint_b = *static_cast (p_b); + return hint_a->get_start_loc () - hint_b->get_start_loc (); +} + /* Implementation of class layout. */ /* Constructor for class layout. @@ -799,6 +809,9 @@ layout::layout (diagnostic_context * context, m_fixit_hints.safe_push (hint); } + /* Sort m_fixit_hints. */ + m_fixit_hints.qsort (fixit_cmp); + /* Populate m_line_spans. */ calculate_line_spans (); @@ -1385,7 +1398,11 @@ layout::annotation_line_showed_range_p (int line, int start_column, struct column_range { - column_range (int start_, int finish_) : start (start_), finish (finish_) {} + column_range (int start_, int finish_) : start (start_), finish (finish_) + { + /* We must have either a range, or an insertion. */ + gcc_assert (start <= finish || finish == start - 1); + } bool operator== (const column_range &other) const { @@ -1427,6 +1444,26 @@ get_printed_columns (const fixit_hint *hint) } } +/* A struct capturing the bounds of a buffer, to allow for run-time + bounds-checking in a checked build. */ + +struct char_span +{ + char_span (const char *ptr, size_t n_elts) : m_ptr (ptr), m_n_elts (n_elts) {} + + char_span subspan (int offset, int n_elts) + { + gcc_assert (offset >= 0); + gcc_assert (offset < (int)m_n_elts); + gcc_assert (n_elts >= 0); + gcc_assert (offset + n_elts <= (int)m_n_elts); + return char_span (m_ptr + offset, n_elts); + } + + const char *m_ptr; + size_t m_n_elts; +}; + /* A correction on a particular line. This describes a plan for how to print one or more fixit_hint instances that affected the line, potentially consolidating hints @@ -1455,6 +1492,14 @@ struct correction void ensure_capacity (size_t len); void ensure_terminated (); + void overwrite (int dst_offset, const char_span &src_span) + { + gcc_assert (dst_offset >= 0); + gcc_assert (dst_offset + src_span.m_n_elts < m_alloc_sz); + memcpy (m_text + dst_offset, src_span.m_ptr, + src_span.m_n_elts); + } + /* If insert, then start: the column before which the text is to be inserted, and finish is offset by the length of the replacement. @@ -1526,6 +1571,26 @@ line_corrections::~line_corrections () delete c; } +/* A struct wrapping a particular source line, allowing + run-time bounds-checking of accesses in a checked build. */ + +struct source_line +{ + source_line (const char *filename, int line); + + char_span as_span () { return char_span (chars, width); } + + const char *chars; + int width; +}; + +/* source_line's ctor. */ + +source_line::source_line (const char *filename, int line) +{ + chars = location_get_source_line (filename, line, &width); +} + /* Add HINT to the corrections for this line. Attempt to consolidate nearby hints so that they will not overlap with printed. */ @@ -1541,6 +1606,14 @@ line_corrections::add_hint (const fixit_hint *hint) { correction *last_correction = m_corrections[m_corrections.length () - 1]; + + /* The following consolidation code assumes that the fix-it hints + have been sorted by start (done within layout's ctor). */ + gcc_assert (affected_columns.start + >= last_correction->m_affected_columns.start); + gcc_assert (printed_columns.start + >= last_correction->m_printed_columns.start); + if (printed_columns.start <= last_correction->m_printed_columns.finish) { /* We have two hints for which the printed forms of the hints @@ -1553,23 +1626,26 @@ line_corrections::add_hint (const fixit_hint *hint) printed_columns.start - 1); /* Try to read the source. */ - int line_width; - const char *line = location_get_source_line (m_filename, m_row, - &line_width); - if (line && between.finish < line_width) + source_line line (m_filename, m_row); + if (line.chars && between.finish < line.width) { /* Consolidate into the last correction: add a no-op "replace" of the "between" text, and add the text from the new hint. */ - size_t old_len = last_correction->m_len; - size_t between_len = between.finish + 1 - between.start; - size_t new_len = old_len + between_len + hint->get_length (); + int old_len = last_correction->m_len; + gcc_assert (old_len >= 0); + int between_len = between.finish + 1 - between.start; + gcc_assert (between_len >= 0); + int new_len = old_len + between_len + hint->get_length (); + gcc_assert (new_len >= 0); last_correction->ensure_capacity (new_len); - memcpy (last_correction->m_text + old_len, - line + between.start - 1, - between.finish + 1 - between.start); - memcpy (last_correction->m_text + old_len + between_len, - hint->get_string (), hint->get_length ()); + last_correction->overwrite + (old_len, + line.as_span ().subspan (between.start - 1, + between.finish + 1 - between.start)); + last_correction->overwrite (old_len + between_len, + char_span (hint->get_string (), + hint->get_length ())); last_correction->m_len = new_len; last_correction->ensure_terminated (); last_correction->m_affected_columns.finish @@ -2791,6 +2867,96 @@ test_overlapped_fixit_printing (const line_table_case &case_) } } +/* Verify that the line_corrections machinery correctly prints + overlapping fixit-hints that have been added in the wrong + order. + Adapted from PR c/81405 seen on gcc.dg/init-excess-1.c*/ + +static void +test_overlapped_fixit_printing_2 (const line_table_case &case_) +{ + /* Create a tempfile and write some text to it. + ...000000000111111111122222222223333333333. + ...123456789012345678901234567890123456789. */ + const char *content + = ("int a5[][0][0] = { 1, 2 };\n"); + temp_source_file tmp (SELFTEST_LOCATION, ".c", content); + line_table_test ltt (case_); + + const line_map_ordinary *ord_map + = linemap_check_ordinary (linemap_add (line_table, LC_ENTER, false, + tmp.get_filename (), 0)); + + linemap_line_start (line_table, 1, 100); + + const location_t final_line_end + = linemap_position_for_line_and_column (line_table, ord_map, 1, 100); + + /* Don't attempt to run the tests if column data might be unavailable. */ + if (final_line_end > LINE_MAP_MAX_LOCATION_WITH_COLS) + return; + + const location_t col_1 + = linemap_position_for_line_and_column (line_table, ord_map, 1, 1); + const location_t col_20 + = linemap_position_for_line_and_column (line_table, ord_map, 1, 20); + const location_t col_21 + = linemap_position_for_line_and_column (line_table, ord_map, 1, 21); + const location_t col_23 + = linemap_position_for_line_and_column (line_table, ord_map, 1, 23); + const location_t col_25 + = linemap_position_for_line_and_column (line_table, ord_map, 1, 25); + + /* Two insertions, in the wrong order. */ + { + rich_location richloc (line_table, col_20); + richloc.add_fixit_insert_before (col_23, "{"); + richloc.add_fixit_insert_before (col_21, "}"); + + /* These fixits should be accepted; they can't be consolidated. */ + ASSERT_EQ (2, richloc.get_num_fixit_hints ()); + const fixit_hint *hint_0 = richloc.get_fixit_hint (0); + ASSERT_EQ (column_range (23, 22), get_affected_columns (hint_0)); + ASSERT_EQ (column_range (23, 23), get_printed_columns (hint_0)); + const fixit_hint *hint_1 = richloc.get_fixit_hint (1); + ASSERT_EQ (column_range (21, 20), get_affected_columns (hint_1)); + ASSERT_EQ (column_range (21, 21), get_printed_columns (hint_1)); + + /* Verify that they're printed correctly. */ + test_diagnostic_context dc; + diagnostic_show_locus (&dc, &richloc, DK_ERROR); + ASSERT_STREQ ("\n" + " int a5[][0][0] = { 1, 2 };\n" + " ^\n" + " } {\n", + pp_formatted_text (dc.printer)); + } + + /* Various overlapping insertions, some occurring "out of order" + (reproducing the fix-it hints from PR c/81405). */ + { + test_diagnostic_context dc; + rich_location richloc (line_table, col_20); + + richloc.add_fixit_insert_before (col_20, "{{"); + richloc.add_fixit_insert_before (col_21, "}}"); + richloc.add_fixit_insert_before (col_23, "{"); + richloc.add_fixit_insert_before (col_21, "}"); + richloc.add_fixit_insert_before (col_23, "{{"); + richloc.add_fixit_insert_before (col_25, "}"); + richloc.add_fixit_insert_before (col_21, "}"); + richloc.add_fixit_insert_before (col_1, "{"); + richloc.add_fixit_insert_before (col_25, "}"); + diagnostic_show_locus (&dc, &richloc, DK_ERROR); + ASSERT_STREQ ("\n" + " int a5[][0][0] = { 1, 2 };\n" + " ^\n" + " { -----\n" + " {{1}}}}, {{{2 }}\n", + pp_formatted_text (dc.printer)); + } +} + /* Insertion fix-it hint: adding a "break;" on a line by itself. */ static void @@ -3001,6 +3167,7 @@ diagnostic_show_locus_c_tests () for_each_line_table_case (test_diagnostic_show_locus_fixit_lines); for_each_line_table_case (test_fixit_consolidation); for_each_line_table_case (test_overlapped_fixit_printing); + for_each_line_table_case (test_overlapped_fixit_printing_2); for_each_line_table_case (test_fixit_insert_containing_newline); for_each_line_table_case (test_fixit_insert_containing_newline_2); for_each_line_table_case (test_fixit_replace_containing_newline); diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index b24aab8ccc7..41dca744045 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,4 +1,9 @@ - 2017-07-13 Will Schmidt +2017-07-13 David Malcolm + + PR c/81405 + * gcc.dg/Wmissing-braces-fixits.c: Add coverage for PR c/81405. */ + +2017-07-13 Will Schmidt * gcc.target/powerpc/fold-vec-missing-lhs.c: New. diff --git a/gcc/testsuite/gcc.dg/Wmissing-braces-fixits.c b/gcc/testsuite/gcc.dg/Wmissing-braces-fixits.c index b7d66cd6c3d..2cce217369c 100644 --- a/gcc/testsuite/gcc.dg/Wmissing-braces-fixits.c +++ b/gcc/testsuite/gcc.dg/Wmissing-braces-fixits.c @@ -162,3 +162,28 @@ struct sa3 arr_4_sa3[4] = \ 6, 7, 8, 9, 10, 11}; {{ }}{{ }} { dg-end-multiline-output "" } */ + +/* PR c/81405. */ +int a5[][0][0] = { 1, 2 }; /* { dg-line pr_81405 } */ + + /* { dg-warning "missing braces around initializer" "" { target c } pr_81405 } */ + /* { dg-begin-multiline-output "" } + int a5[][0][0] = { 1, 2 }; + ^ + { ----- + {{1}}}}, {{{2 }} + { dg-end-multiline-output "" } */ + + /* { dg-warning "excess elements" "" { target c } pr_81405 } */ + /* { dg-begin-multiline-output "" } + int a5[][0][0] = { 1, 2 }; + ^ + { dg-end-multiline-output "" } */ + /* { dg-begin-multiline-output "" } + int a5[][0][0] = { 1, 2 }; + ^ + { dg-end-multiline-output "" } */ + /* { dg-begin-multiline-output "" } + int a5[][0][0] = { 1, 2 }; + ^~~ + { dg-end-multiline-output "" } */ -- 2.30.2