Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Andrew Burgess <aburgess@redhat.com>
To: Tom Tromey <tromey@adacore.com>, gdb-patches@sourceware.org
Cc: Tom Tromey <tromey@adacore.com>
Subject: Re: [PATCH] Always fetch Ada "main" name from the executable
Date: Wed, 05 Aug 2026 18:07:43 +0100	[thread overview]
Message-ID: <87qzkclcm8.fsf@redhat.com> (raw)
In-Reply-To: <20260730170336.2547536-1-tromey@adacore.com>

Tom Tromey <tromey@adacore.com> writes:

> The gdb.ada/file-then-restart.exp test was failing with gnat-llvm.  I
> tracked this down to the "main" name not being stored in a readonly
> section, meaning that the code in ada_main_name using trust_readonly
> did not work.
>
> However, it seems to me that gdb should always prefer the data from
> the executable in this particular case.  So, rather than relying on
> trust_readonly, this patch changes gdb to do this directly.

I was pointed at this:

  https://sourceware.org/pipermail/bunsen/2026q3/001484.html

It's an AI/LLM code review of this patch.  The review in this case seems
to be totally bogus, everything it is commenting on is either fine, or
is part of the documented API of the function.

However, I asked Claude to review the patch and it did highlight one
issue.  It's mostly theoretical, but fixing it is trivial, so we might
as well.

Let me know what you think.

Thanks,
Andrew

---

commit 5990efb77092a3a02330260b88899d5a8b15bca7
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Aug 5 17:23:38 2026 +0100

    gdb/ada: avoid rereading stale main name data in edge case
    
    The commit:
    
      commit 8eafbbc74748e499ec785f78858687bd7ea79005
      Date:   Wed Jul 29 12:40:03 2026 -0600
    
        Always fetch Ada "main" name from the executable
    
    changes ada_main_name to use section_table_xfer_memory_partial.  This
    introduced a highly unlikely, but theoretical bug where stale buffer
    data could cause GDB to find an invalid name for "main".
    
    Looking at ada_main_name (in ada-lang.c), the steps to reproduce the
    bug are:
    
      1. Debug a program that causes the static buffer main_program_name
         to have some content written to it.  For the sake of this bug
         let's assume the main name is "xxxxxxxxxx", the main_program_name
         buffer will contain 10 'x' characters, a null byte, then whatever
         happened to be in the section after that.
    
      2. A new executable is loaded into GDB and ada_main_name is called
         again.
    
      3. For whatever reason the new executable is maybe not correct.  The
         ADA_MAIN_PROGRAM_SYMBOL_NAME symbol points to an address 5 bytes
         before the end of a section.  None of these 5 bytes are a null
         bytes.  Let's assume these 5 bytes are "aaaaa".
    
      4. The section_table_xfer_memory_partial call will try to read up to
         1024 bytes, but as there are only 5 bytes left in the section,
         only 5 will be read.  This leaves the main_program_name buffer
         containing "aaaaaxxxxx" followed by a null character byte.
    
      5. GDB returns this merged string as the result from ada_main_name.
    
    Now given this depends on the second executable being broken, we maybe
    don't really care too much, however, fixing this is pretty easy.
    
    The current code already checks:
    
      && (strnlen ((char *) main_program_name, sizeof (main_program_name))
          < sizeof (main_program_name))
    
    This ensures that there's a string with a null byte contained within
    the buffer, but makes the assumption that we always read
    sizeof (main_program_name) bytes from the section.
    
    But we know how many bytes were read, that's the value in XFERRED.
    What we really want to ask is: was there a null terminated string
    within the bytes that we just read.  This is:
    
      && (strnlen ((char *) main_program_name, xferred) < xferred)
    
    Given how simple this fix is, let's make it.

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 906c5cd3465..174e04af04c 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -806,8 +806,7 @@ ada_main_name ()
 					      sections)
 	   == TARGET_XFER_OK)
 	  && xferred > 0
-	  && (strnlen ((char *) main_program_name, sizeof (main_program_name))
-	      < sizeof (main_program_name)))
+	  && (strnlen ((char *) main_program_name, xferred) < xferred))
 	return (char *) main_program_name;
     }
 


  parent reply	other threads:[~2026-08-05 17:08 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-30 17:03 Tom Tromey
2026-07-31 10:48 ` Pedro Alves
2026-08-05 17:07 ` Andrew Burgess [this message]
2026-08-07 13:55   ` Tom Tromey
2026-08-07 15:03     ` Andrew Burgess

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87qzkclcm8.fsf@redhat.com \
    --to=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=tromey@adacore.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox