[Bug gdb/27847] New: A possible divide by zero bug in amd64_has_unaligned_fields (amd64-tdep.c)

[Bug gdb/27847] New: A possible divide by zero bug in amd64_has_unaligned_fields (amd64-tdep.c)

[yguoaz at cse dot ust.hk]

https://sourceware.org/bugzilla/show_bug.cgi?id=27847

            Bug ID: 27847
           Summary: A possible divide by zero bug in
                    amd64_has_unaligned_fields (amd64-tdep.c)
           Product: gdb
           Version: HEAD
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: gdb
          Assignee: unassigned at sourceware dot org
          Reporter: yguoaz at cse dot ust.hk
  Target Milestone: ---

In the file gdb/amd64-tdep.c, the function amd64_has_unaligned_fields has the
following code (link to the code location:
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/amd64-tdep.c;h=66a7c02f53403a608122d677f5bd14aeb8df74ee;hb=2005aa0281fccd53ad867ead783aa417516cf39c#l557):

static bool
amd64_has_unaligned_fields (struct type *type)
{
    if (type->code () == TYPE_CODE_STRUCT
      || type->code () == TYPE_CODE_UNION) {
        ...
        int align = type_align(subtype);
        ...
        if (bytepos % align != 0)
            return true;
    }
}

The function type_align may return zero, leading to a potential divide by zero
problem in the modulo operation.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/27847] A possible divide by zero bug in amd64_has_unaligned_fields (amd64-tdep.c)

[yguoaz at cse dot ust.hk]

https://sourceware.org/bugzilla/show_bug.cgi?id=27847

YiyuanGUO <yguoaz at cse dot ust.hk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |palves at redhat dot com,
                   |                            |yguoaz at cse dot ust.hk

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/27847] A possible divide by zero bug in amd64_has_unaligned_fields (amd64-tdep.c)

[yguoaz at cse dot ust.hk]

https://sourceware.org/bugzilla/show_bug.cgi?id=27847

YiyuanGUO <yguoaz at cse dot ust.hk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |brobecke at sources dot redhat.com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/27847] A possible divide by zero bug in amd64_has_unaligned_fields (amd64-tdep.c)

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27847

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tromey at sourceware dot org

--- Comment #1 from Tom Tromey <tromey at sourceware dot org> ---
It can return 0 in theory, but do you have a test case where it
does in practice?  That would be helpful.
I think under normal circumstances it should not return 0.
You may need a test case in Pascal (which may not work anyway)
or perhaps intentionally corrupted DWARF.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/27847] A possible divide by zero bug in amd64_has_unaligned_fields (amd64-tdep.c)

[yguoaz at cse dot ust.hk]

https://sourceware.org/bugzilla/show_bug.cgi?id=27847

--- Comment #2 from YiyuanGUO <yguoaz at cse dot ust.hk> ---
(In reply to Tom Tromey from comment #1)
> It can return 0 in theory, but do you have a test case where it
> does in practice?  That would be helpful.
> I think under normal circumstances it should not return 0.
> You may need a test case in Pascal (which may not work anyway)
> or perhaps intentionally corrupted DWARF.

Hi, this is a bug detected by static analysis and I currently do not have a
POC. But I think it is reasonable and easy enough to add some simple protection
to avoid any potential problems. One possible fix is:

static bool
amd64_has_unaligned_fields (struct type *type)
{
  if (type->code () == TYPE_CODE_STRUCT
      || type->code () == TYPE_CODE_UNION)
    {
      for (int i = 0; i < type->num_fields (); i++)
        {
          struct type *subtype = check_typedef (type->field (i).type ());
          int bitpos = TYPE_FIELD_BITPOS (type, i);
          int align = type_align(subtype);

          if (!align) // Ignore field with 0 alignment
            continue;

          /* Ignore static fields, empty fields (for example nested
             empty structures), and bitfields (these are handled by
             the caller).  */
          if (field_is_static (&type->field (i))
              || (TYPE_FIELD_BITSIZE (type, i) == 0
                  && TYPE_LENGTH (subtype) == 0)
              || TYPE_FIELD_PACKED (type, i))
            continue;

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/27847] A possible divide by zero bug in amd64_has_unaligned_fields (amd64-tdep.c)

[andrew.burgess at embecosm dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27847

Andrew Burgess <andrew.burgess at embecosm dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrew.burgess at embecosm dot com

--- Comment #3 from Andrew Burgess <andrew.burgess at embecosm dot com> ---
Created attachment 13442
  --> https://sourceware.org/bugzilla/attachment.cgi?id=13442&action=edit
possible test and fix

The attachment contains a possible test for this issue as well as a possible
fix.  

Unlike the previously proposed fix I suggest that if 0 ever comes up then we
throw an error.  The 0 indicates that GDB is unable to figure out the alignment
for a field, as such I don't think we can correctly answer the question as to
whether the struct has any unaligned fields, and answer we give would be a
guess, and thus might lead to incorrect results.

Let me know your thoughts.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/27847] A possible divide by zero bug in amd64_has_unaligned_fields (amd64-tdep.c)

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27847

--- Comment #4 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Andrew Burgess from comment #3)
> Created attachment 13442 [details]
> possible test and fix
> 
> The attachment contains a possible test for this issue as well as a possible
> fix.  

It seems reasonable to me.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/27847] A possible divide by zero bug in amd64_has_unaligned_fields (amd64-tdep.c)

[andrew.burgess at embecosm dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27847

--- Comment #5 from Andrew Burgess <andrew.burgess at embecosm dot com> ---
Patch on the list:
  https://sourceware.org/pipermail/gdb-patches/2021-June/179534.html

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/27847] A possible divide by zero bug in amd64_has_unaligned_fields (amd64-tdep.c)

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27847

--- Comment #6 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Andrew Burgess <aburgess@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a12a15e7c5ff9f3549dd185fa1acd5a9dddd001b

commit a12a15e7c5ff9f3549dd185fa1acd5a9dddd001b
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Wed May 12 13:44:06 2021 +0100

    gdb: handle case where type alignment is unknown

    It was spotted that if type_align returned 0 then it was possible to
    trigger a divide by zero exception within GDB.  It turns out this will
    only happen in an edge case where GDB is unable to figure out the
    alignment of a field within a structure.

    The attached test generates some non-standard, probably broken, DWARF,
    that triggers this condition, and then fixes this issue by throwing an
    exception when this case occurs.

    gdb/ChangeLog:

            PR gdb/27847
            * amd64-tdep.c (amd64_has_unaligned_fields): Move call to
            type_align, and spot case where the alignment is unknown.

    gdb/testsuite/ChangeLog:

            PR gdb/27847
            * gdb.dwarf2/dw2-weird-type-len.c: New file.
            * gdb.dwarf2/dw2-weird-type-len.exp: New file.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/27847] A possible divide by zero bug in amd64_has_unaligned_fields (amd64-tdep.c)

[andrew.burgess at embecosm dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=27847

Andrew Burgess <andrew.burgess at embecosm dot com> changed:

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

--- Comment #7 from Andrew Burgess <andrew.burgess at embecosm dot com> ---
I believe this issue is now resolved.

-- 
You are receiving this mail because:
You are on the CC list for the bug.