[Bug gdb/15161] New: symfile.c:struct load_section_data::load_offset is "unsigned long" but should be wider.

[Bug gdb/15161] New: symfile.c:struct load_section_data::load_offset is "unsigned long" but should be wider.

[palves at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15161

             Bug #: 15161
           Summary: symfile.c:struct load_section_data::load_offset is
                    "unsigned long" but should be wider.
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: gdb
        AssignedTo: unassigned@sourceware.org
        ReportedBy: palves@redhat.com
    Classification: Unclassified


symfile.c has:

/* Opaque data for load_section_callback.  */
struct load_section_data {
  unsigned long load_offset;
  struct load_progress_data *progress_data;
  VEC(memory_write_request_s) *requests;
};

static void
load_section_callback (bfd *abfd, asection *asec, void *data)
{
  struct memory_write_request *new_request;
...
  new_request->begin = bfd_section_lma (abfd, asec) + args->load_offset;
...

void
generic_load (char *args, int from_tty)
{
...
      cbdata.load_offset = strtoul (argv[1], &endptr, 0);


"unsigned long" is not wide enough in the cases of 32-bit gdb x 64-bit target,
or LLP64 (like Windows 64-bit).  load_offset should be ULONGEST or CORE_ADDR.

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug gdb/15161] symfile.c:struct load_section_data::load_offset is "unsigned long" but should be wider.

[palves at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15161

--- Comment #1 from Pedro Alves <palves at redhat dot com> 2013-02-19 16:19:50 UTC ---
Kai mentions monitor.c has a similar issue in monitor_load().

Other xxx_load functions should be audited too.

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug gdb/15161] symfile.c:struct load_section_data::load_offset is "unsigned long" but should be wider.

[cvs-commit at gcc dot gnu.org]

http://sourceware.org/bugzilla/show_bug.cgi?id=15161

--- Comment #2 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> 2013-02-19 18:32:02 UTC ---
CVSROOT:    /cvs/src
Module name:    src
Changes by:    ktietz@sourceware.org    2013-02-19 18:31:50

Modified files:
    gdb            : ChangeLog symfile.c 

Log message:
    PR gdb/15161
    * symfile.c (load_section_data): Change type of load_offset
    to CORE_ADDR.
    (generic_load): User strtoulst instead of strtoul for conversion
    of load_offset.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/ChangeLog.diff?cvsroot=src&r1=1.15169&r2=1.15170
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/symfile.c.diff?cvsroot=src&r1=1.363&r2=1.364

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug gdb/15161] symfile.c:struct load_section_data::load_offset is "unsigned long" but should be wider.

[cvs-commit at gcc dot gnu.org]

http://sourceware.org/bugzilla/show_bug.cgi?id=15161

--- Comment #3 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> 2013-02-19 19:27:32 UTC ---
CVSROOT:    /cvs/src
Module name:    src
Changes by:    palves@sourceware.org    2013-02-19 19:27:22

Modified files:
    gdb            : ChangeLog monitor.c 

Log message:
    Harmonize this monitor_load with generic_load.

    Harmonize this old-looking code with generic_load, which fixes several
    issues.

    2013-02-19  Pedro Alves  <palves@redhat.com>

    PR gdb/15161

    Harmonize with generic_load.

    * monitor.c: Include "readline/readline.h".
    (monitor_load): Rename parameter 'file' to 'args'.  Use build_argv
    instead of sscanf.  Use CORE_ADDR/strtoulst instead of unsigned
    long/strtol for the 'load_offset' local.  Error out if no argument
    is given or if too many arguments are given.  Tilde expand the
    passed in file name.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/ChangeLog.diff?cvsroot=src&r1=1.15170&r2=1.15171
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/monitor.c.diff?cvsroot=src&r1=1.113&r2=1.114

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug gdb/15161] symfile.c:struct load_section_data::load_offset is "unsigned long" but should be wider.

[cvs-commit at gcc dot gnu.org]

http://sourceware.org/bugzilla/show_bug.cgi?id=15161

--- Comment #4 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> 2013-02-19 20:53:04 UTC ---
CVSROOT:    /cvs/src
Module name:    src
Changes by:    palves@sourceware.org    2013-02-19 20:52:58

Modified files:
    gdb/gdbserver  : ChangeLog server.c 

Log message:
    gdbserver:server.c - use unpack_varlen_hex to extract hex numbers.

    Addresses, as most numbers in the RSP are hex encoded, with variable
    length (that just means the width isn't specified, and there's no top
    cap.  So they should be extracted with unpack_varlen_hex.

    A couple spots in server.c are using strto(u)l, which doesn't work on
    LLP64 targets.

    This patch fixes it.

    Tested on x86_64 Fedora 17.

    2013-02-19  Pedro Alves  <palves@redhat.com>
    Kai Tietz <ktietz@redhat.com>

    PR gdb/15161

    * server.c (handle_query) <CRC check>: Use unpack_varlen_hex
    instead of strtoul to extract address from packet.
    (process_serial_event) <'z'>: Likewise.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/gdbserver/ChangeLog.diff?cvsroot=src&r1=1.681&r2=1.682
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/gdbserver/server.c.diff?cvsroot=src&r1=1.184&r2=1.185

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug gdb/15161] symfile.c:struct load_section_data::load_offset is "unsigned long" but should be wider.

[palves at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15161

--- Comment #5 from Pedro Alves <palves at redhat dot com> 2013-02-19 20:57:30 UTC ---
remote-mips.c:mips_load calls pmon_load_fast which has:

static void
pmon_load_fast (char *file)
{
...
    printf_filtered ("%s\t: 0x%4x .. 0x%4x  ", s->name,
             (unsigned int) s->vma,
             (unsigned int) (s->vma + bfd_get_section_size (s)));

This printf truncates vma's to 32-bit.

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug gdb/15161] symfile.c:struct load_section_data::load_offset is "unsigned long" but should be wider.

[palves at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15161

--- Comment #6 from Pedro Alves <palves at redhat dot com> 2013-02-19 20:59:16 UTC ---
I audited all target_ops::to_load implementations, and didn't see any other
similar issue.

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug gdb/15161] symfile.c:struct load_section_data::load_offset is "unsigned long" but should be wider.

[palves at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15161

--- Comment #7 from Pedro Alves <palves at redhat dot com> 2013-02-19 21:11:00 UTC ---
And looks weirdly buggy.  "0x%4x" results in printing things like

"filename<tab>: 0x   1 .. 0x   2  "

It must be that "0x%04x" was meant, so we'd get "0x0001", "0x0002"...

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug gdb/15161] symfile.c:struct load_section_data::load_offset is "unsigned long" but should be wider.

[palves at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15161

--- Comment #8 from Pedro Alves <palves at redhat dot com> 2013-02-19 21:20:08 UTC ---
Alright, I almost committed a patch for that, but I noticed that
the "final" local seen on the patch is an "int", and following that
local's usage we see lots of 32-bit assumption.  So I'll just leave
the code as is.

commit 023172580ad3b79cec08ee48fa422ba4cbbcfaa5
Author: Pedro Alves <palves@redhat.com>
Date:   Tue Feb 19 20:59:31 2013 +0000

    Another bit of gdb/15161 - remote-mips.c

    This printf truncates vma's to 32-bit.  I don't know if this is used or
reacheable
    with MIPS64 at all, but as long as I noticed it, better make it follow
    gdb's conventions.

    Actually, the existing code looks weirdly buggy.  "0x%4x" results in
printing things like

     "filename<tab>: 0x   1 .. 0x   2  "

    It must be that "0x%04x" was meant, so we'd get instead:

     "filename<tab>: 0x0001 .. 0x0002  "

    phex does that.

    Tested by building with --enable-targets=all.

    2013-02-19  Pedro Alves  <palves@redhat.com>

        * remote-mips.c (pmon_load_fast): Use phex with target's address
        width instead of "0x%4x" format for printing addresses.

diff --git a/gdb/remote-mips.c b/gdb/remote-mips.c
index e20a740..8457890 100644
--- a/gdb/remote-mips.c
+++ b/gdb/remote-mips.c
@@ -3373,6 +3373,7 @@ pmon_load_fast (char *file)
   int final = 0;
   int finished = 0;
   struct cleanup *cleanup;
+  int addr_size = gdbarch_addr_bit (target_gdbarch ()) / 8;

   buffer = (char *) xmalloc (MAXRECSIZE + 1);
   binbuf = (unsigned char *) xmalloc (BINCHUNK);
@@ -3413,9 +3414,10 @@ pmon_load_fast (char *file)
     bintotal += bfd_get_section_size (s);
     final = (s->vma + bfd_get_section_size (s));

-    printf_filtered ("%s\t: 0x%4x .. 0x%4x  ", s->name,
-             (unsigned int) s->vma,
-             (unsigned int) (s->vma + bfd_get_section_size (s)));
+    printf_filtered ("%s\t: 0x%s .. 0x%s  ", s->name,
+             phex (s->vma, addr_size),
+             phex (s->vma + bfd_get_section_size (s), addr_size));
+
     gdb_flush (gdb_stdout);

     /* Output the starting address.  */

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug gdb/15161] symfile.c:struct load_section_data::load_offset is "unsigned long" but should be wider.

[palves at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15161

Pedro Alves <palves at redhat dot com> changed:

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

--- Comment #9 from Pedro Alves <palves at redhat dot com> 2013-02-19 21:21:11 UTC ---
> I audited all target_ops::to_load implementations, and didn't see any other similar issue.

Alright, marking fixed.

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.