[Bug analyzer/111537] New: ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[Bug analyzer/111537] New: ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

            Bug ID: 111537
           Summary: ICE: in set_cell_span, at text-art/table.cc:148 with D
                    front-end and -fanalyzer
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: ibuclaw at gdcproject dot org
  Target Milestone: ---

Roughly copied an example from the static analyzer talk and wrote it in D.
---
import core.stdc.string;
void main()
{
    char[5] arr;
    strcpy(arr.ptr, "hello world");
}
---


The C++ equivalent compiled with g++ prints pretty tables and emojis in the
expected warning, whereas with gdc compiler issues an ICE within the text-art
code.

---
oob.d: In function ‘D main’:
oob.d:5:11: warning: buffer over-read [CWE-126] [-Wanalyzer-out-of-bounds]
    5 |     strcpy(arr.ptr, "hello world");
      |           ^
  ‘D main’: events 1-3
    |
    |    4 |     char[5] arr;
    |      |             ^
    |      |             |
    |      |             (1) following ‘false’ branch...
    |      |             (2) ...to here
    |    5 |     strcpy(arr.ptr, "hello world");
    |      |           ~  
    |      |           |
    |      |           (3) out-of-bounds read at byte 11 but ‘"hello world"’
ends at byte 11
    |
oob.d:5:11: note: read of 1 byte from after the end of ‘"hello world"’
    5 |     strcpy(arr.ptr, "hello world");
      |           ^
oob.d:5:11: note: valid subscripts for ‘"hello world"’ are ‘[0]’ to ‘[11]’
during IPA pass: analyzer
oob.d:4:13: internal compiler error: in set_cell_span, at text-art/table.cc:148
    4 |     char[5] arr;
      |             ^
0xa837bf text_art::table::set_cell_span(text_art::rect<text_art::table>,
text_art::table_cell_content&&, text_art::x_align, text_art::y_align)
        ../../gcc/text-art/table.cc:148
0x2404913
ana::access_diagram_impl::add_invalid_accesses_to_region_table(text_art::table&)
        ../../gcc/analyzer/access-diagram.cc:2025
0x240b826 ana::access_diagram_impl::access_diagram_impl(ana::access_operation
const&, diagnostic_event_id_t, text_art::style_manager&, text_art::theme
const&, ana::logger*)
        ../../gcc/analyzer/access-diagram.cc:1874
0x23ffccb std::enable_if<!std::is_array<ana::access_diagram_impl>::value,
std::unique_ptr<ana::access_diagram_impl,
std::default_delete<ana::access_diagram_impl> > >::type
make_unique<ana::access_diagram_impl, ana::access_operation const&,
diagnostic_event_id_t&, text_art::style_manager&, text_art::theme const&,
ana::logger*&>(ana::access_operation const&, diagnostic_event_id_t&,
text_art::style_manager&, text_art::theme const&, ana::logger*&)
        ../../gcc/make-unique.h:41
0x23ffccb ana::access_diagram::access_diagram(ana::access_operation const&,
diagnostic_event_id_t, text_art::style_manager&, text_art::theme const&,
ana::logger*)
        ../../gcc/analyzer/access-diagram.cc:2416
0x21db92a ana::out_of_bounds::make_access_diagram(ana::access_operation const&,
text_art::style_manager&, text_art::theme const&, ana::logger*) const
        ../../gcc/analyzer/bounds-checking.cc:190
0x21db92a ana::out_of_bounds::maybe_show_diagram(ana::logger*) const
        ../../gcc/analyzer/bounds-checking.cc:169
0x21dbf7f ana::out_of_bounds::maybe_show_notes(unsigned int, ana::logger*)
const
        ../../gcc/analyzer/bounds-checking.cc:125
0x21dbf7f ana::concrete_buffer_over_read::emit(rich_location*, ana::logger*)
        ../../gcc/analyzer/bounds-checking.cc:437
0x220168c ana::diagnostic_manager::emit_saved_diagnostic(ana::exploded_graph
const&, ana::saved_diagnostic&)
        ../../gcc/analyzer/diagnostic-manager.cc:1566
0x220544d ana::dedupe_winners::emit_best(ana::diagnostic_manager*,
ana::exploded_graph const&)
        ../../gcc/analyzer/diagnostic-manager.cc:1446
0x2201c2e ana::diagnostic_manager::emit_saved_diagnostics(ana::exploded_graph
const&)
        ../../gcc/analyzer/diagnostic-manager.cc:1498
0x16771e5 ana::impl_run_checkers(ana::logger*)
        ../../gcc/analyzer/engine.cc:6168
0x16780ee ana::run_checkers()
        ../../gcc/analyzer/engine.cc:6242
0x166738c execute
        ../../gcc/analyzer/analyzer-pass.cc:87

[Bug analyzer/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Am trying to reproduce locally, but when I run this in my BUILDDIR/gcc:
  ./gdc -B. -S -fanalyzer oob.d 
I get:
  d21: error: cannot find source code for runtime library file 'object.d'

Possibly a silly question, but what am I doing wrong?

[Bug analyzer/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

--- Comment #2 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
(In reply to David Malcolm from comment #1)
> Am trying to reproduce locally, but when I run this in my BUILDDIR/gcc:
>   ./gdc -B. -S -fanalyzer oob.d 
> I get:
>   d21: error: cannot find source code for runtime library file 'object.d'
> 
> Possibly a silly question, but what am I doing wrong?

The library sources aren't in any paths that the compiler looks for.  It should
work without any explicit `-I` if the target libraries were installed.

This alternative reproducer should get you off the ground though.
---

mkdir gcc;
echo "module gcc.builtins;" > gcc/builtins.d
cat > oob.d << EOF
module object;
import gcc.builtins;
void main()
{
    char[5] arr;
    __builtin_strcpy(arr.ptr, "hello world");
}
EOF
./gdc -B. -S -fanalyzer oob.d

[Bug analyzer/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-10-11
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks; that reproducer works for me.

[Bug analyzer/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

--- Comment #4 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(In reply to David Malcolm from comment #3)
> Thanks; that reproducer works for me.
...or rather, demonstrates the ICE in a way that I can see in the debugger.

[Bug analyzer/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
It's complaining about the read from the string literal.

If I change the string in the reproducer from "hello world" to "foo", I see:

(gdb) pt string_cst
 <string_cst 0x7fffea76f738
    type <array_type 0x7fffea78a150
        type <integer_type 0x7fffea772930 char readonly unsigned QI
            size <integer_cst 0x7fffea644eb8 constant 8>
            unit-size <integer_cst 0x7fffea644ed0 constant 1>
            align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7fffea772930 precision:8 min <integer_cst 0x7fffea6624c8 0> max <integer_cst
0x7fffea662450 255>
            pointer_to_this <pointer_type 0x7fffea78a000>>
        SI
        size <integer_cst 0x7fffea662018 constant 32>
        unit-size <integer_cst 0x7fffea662030 constant 4>
        align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7fffea78a150
        domain <integer_type 0x7fffea672150 type <integer_type 0x7fffea65e000
sizetype>
            DI
            size <integer_cst 0x7fffea644dc8 constant 64>
            unit-size <integer_cst 0x7fffea644de0 constant 8>
            align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7fffea672150 precision:64 min <integer_cst 0x7fffea644df8 0> max <integer_cst
0x7fffea662558 3>>
        pointer_to_this <p

Looking at TREE_STRING_LENGTH:

/* In a STRING_CST */
/* In C terms, this is sizeof, not strlen.  */
#define TREE_STRING_LENGTH(NODE) (STRING_CST_CHECK (NODE)->string.length)

(gdb) p string_cst->string.length
$36 = 3

The analyzer is using this for determining the validly accessible size of the
string, which it determines is 3 bytes:

(gdb) call valid_bits.dump(true)
bytes 0-2

whereas the read is of 4 bytes:
(gdb) call actual_bits.dump(true)
bytes 0-3

Is D correctly building that string_cst?  Are D strings 0-terminated?

[Bug analyzer/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

--- Comment #6 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Oops; the above got truncated; the string_cst prints as follows in gdb

(gdb) pt string_cst
 <string_cst 0x7fffea76f738
    type <array_type 0x7fffea78a150
        type <integer_type 0x7fffea772930 char readonly unsigned QI
            size <integer_cst 0x7fffea644eb8 constant 8>
            unit-size <integer_cst 0x7fffea644ed0 constant 1>
            align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7fffea772930 precision:8 min <integer_cst 0x7fffea6624c8 0> max <integer_cst
0x7fffea662450 255>
            pointer_to_this <pointer_type 0x7fffea78a000>>
        SI
        size <integer_cst 0x7fffea662018 constant 32>
        unit-size <integer_cst 0x7fffea662030 constant 4>
        align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7fffea78a150
        domain <integer_type 0x7fffea672150 type <integer_type 0x7fffea65e000
sizetype>
            DI
            size <integer_cst 0x7fffea644dc8 constant 64>
            unit-size <integer_cst 0x7fffea644de0 constant 8>
            align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7fffea672150 precision:64 min <integer_cst 0x7fffea644df8 0> max <integer_cst
0x7fffea662558 3>>
        pointer_to_this <pointer_type 0x7fffea78a1f8>>
    constant "foo">

[Bug analyzer/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

--- Comment #7 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
(In reply to David Malcolm from comment #5)
> Is D correctly building that string_cst?  Are D strings 0-terminated?
Yes, D strings are 0-terminated.

The way I've done it is, the string is constructed using

  build_string(3, "foo")

but the TREE_TYPE is set to `char[4]` so that the terminating 0 is considered.

I'm sure it's like this because either GCC has a tendency to emit strings
side-by-side without the 0 so strlen("foo") could return 20, or there's an
optimization pass for strlen() that would otherwise evaluate strlen("foo") into
2 if I set the type of the literal as `char[3]`.

[Bug analyzer/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

--- Comment #8 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
Looking at C++ FE, I see they construct the string literal using

  build_string (4, "foo")

because I can see the terminating 0 in the pretty-printed string.

---
 <string_cst 0x7ffff717b988
    type <array_type 0x7ffff717d0a8
        type <integer_type 0x7ffff743b5e8 char readonly sizes-gimplified type_6
QI
            size <integer_cst 0x7ffff742a258 constant 8>
            unit-size <integer_cst 0x7ffff742a270 constant 1>
            align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7ffff743b5e8 precision:8 min <integer_cst 0x7ffff742a2a0 -128> max
<integer_cst 0x7ffff742a2d0 127>
            pointer_to_this <pointer_type 0x7ffff743b690>>
        type_6 BLK
        size <integer_cst 0x7ffff75ec408 constant 96>
        unit-size <integer_cst 0x7ffff75ec120 constant 12>
        align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7ffff717d0a8
        domain <integer_type 0x7ffff717de70 type <integer_type 0x7ffff7430000
sizetype>
            type_6 DI
            size <integer_cst 0x7ffff742a168 constant 64>
            unit-size <integer_cst 0x7ffff742a180 constant 8>
            align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7ffff717de70 precision:64 min <integer_cst 0x7ffff742a198 0> max <integer_cst
0x7ffff75ec288 11>>
        pointer_to_this <pointer_type 0x7ffff717d498>>
    readonly constant static "hello world\000">
---


I see in the olden days when D sat outside of GCC, this is what was done too.

https://github.com/D-Programming-GDC/gdc/commit/b9d36fc9d71ec4122d1c986599d87c6cb91ca55c

This might have been accidentally removed by another contributor in an apparent
drive-by search and replace of a specific pattern.

https://github.com/D-Programming-GDC/gdc/commit/f8a05f41cb756ef7ed2b1d8b1d33eb8f815421a5

[Bug analyzer/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[ibuclaw at gdcproject dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

--- Comment #9 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
(In reply to Iain Buclaw from comment #8)
> I see in the olden days when D sat outside of GCC, this is what was done too.
> 
> https://github.com/D-Programming-GDC/gdc/commit/
> b9d36fc9d71ec4122d1c986599d87c6cb91ca55c
Although thinking it over, that did not take into consideration wide character
literals.  The number of 0's at the end of the raw string in STRING_CST nodes
would need to be the same as the width of the type it's representing - one,
two, or four 0's for char, wchar and dchar respectively.

[Bug d/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[ibuclaw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

ibuclaw at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ibuclaw at gcc dot gnu.org
           Assignee|dmalcolm at gcc dot gnu.org        |ibuclaw at gcc dot gnu.org
          Component|analyzer                    |d

--- Comment #10 from ibuclaw at gcc dot gnu.org ---
Nice, thanks for pointing me to specifically where analyzer was tripping over
David.

I think I can take over this PR from here.

---

oob.d: In function ‘D main’:
oob.d:5:11: warning: stack-based buffer overflow [CWE-121]
[-Wanalyzer-out-of-bounds]
    5 |     strcpy(arr.ptr, "hello world");
      |           ^
  ‘D main’: events 1-4
    |
    |    4 |     char[5] arr;
    |      |             ^
    |      |             |
    |      |             (1) capacity: 5 bytes
    |      |             (2) following ‘false’ branch...
    |      |             (3) ...to here
    |    5 |     strcpy(arr.ptr, "hello world");
    |      |           ~  
    |      |           |
    |      |           (4) out-of-bounds write from byte 5 till byte 11 but
‘arr’ ends at byte 5
    |
oob.d:5:11: note: write of 7 bytes to beyond the end of ‘arr’
    5 |     strcpy(arr.ptr, "hello world");
      |           ^
oob.d:5:11: note: valid subscripts for ‘arr’ are ‘[0]’ to ‘[4]’

  ┌─────┬─────┬─────┬─────┬───────┐┌─────┬─────┬─────┬────┬────┬────┬────┐
  │ [0] │ [1] │ [2] │ [3] │  [4]  ││ [5] │ [6] │ [7] │[8] │[9] │[10]│[11]│
  ├─────┼─────┼─────┼─────┼───────┤├─────┼─────┼─────┼────┼────┼────┼────┤
  │ ‘h’ │ ‘e’ │ ‘l’ │ ‘l’ │  ‘o’  ││ ‘ ’ │ ‘w’ │ ‘o’ │‘r’ │‘l’ │‘d’ │NUL │
  ├─────┴─────┴─────┴─────┴───────┴┴─────┴─────┴─────┴────┴────┴────┴────┤
  │               string literal (type: ‘const char[12]’)                │
  └──────────────────────────────────────────────────────────────────────┘
     │     │     │     │      │       │     │     │    │    │    │    │
     │     │     │     │      │       │     │     │    │    │    │    │
     v     v     v     v      v       v     v     v    v    v    v    v
  ┌─────┬─────────────────┬───────┐┌─────────────────────────────────────┐
  │ [0] │       ...       │  [4]  ││                                     │
  ├─────┴─────────────────┴───────┤│          after valid range          │
  │    ‘arr’ (type: ‘char[5]’)    ││                                     │
  └───────────────────────────────┘└─────────────────────────────────────┘
  ├───────────────┬───────────────┤├──────────────────┬──────────────────┤
                  │                                   │
         ╭────────┴────────╮              ╭───────────┴──────────╮
         │capacity: 5 bytes│              │⚠️  overflow of 7 bytes│
         ╰─────────────────╯              ╰──────────────────────╯

[Bug d/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain Buclaw <ibuclaw@gcc.gnu.org>:

https://gcc.gnu.org/g:578afbc751d122b55196a23fe75a17e1b4e9bd0c

commit r14-4639-g578afbc751d122b55196a23fe75a17e1b4e9bd0c
Author: Iain Buclaw <ibuclaw@gdcproject.org>
Date:   Sat Oct 14 02:19:41 2023 +0200

    Fix ICE in set_cell_span, at text-art/table.cc:148 with D front-end and
-fanalyzer

    The internal error in analyzer turned out to be caused by a subtly
    invalid tree representation of STRING_CSTs in the D front-end, fixed by
    including the terminating NULL as part of the TREE_STRING_POINTER.

    When adding a first analyzer test for D, it flagged up another subtle
    mismatch in one assignment in the module support routines as well, fixed
    by generating the correct field type for the compiler-generated struct.

            PR d/111537

    gcc/d/ChangeLog:

            * expr.cc (ExprVisitor::visit (StringExp *)): Include null
terminator
            in STRING_CST string.
            * modules.cc (get_compiler_dso_type): Generate ModuleInfo** type
for
            the minfo fields.

    gcc/testsuite/ChangeLog:

            * gdc.dg/analyzer/analyzer.exp: New test.
            * gdc.dg/analyzer/pr111537.d: New test.

[Bug d/111537] ICE: in set_cell_span, at text-art/table.cc:148 with D front-end and -fanalyzer

[ibuclaw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111537

ibuclaw at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #12 from ibuclaw at gcc dot gnu.org ---
Fixes committed. Thanks!