diff mbox series

[2/3] gdb/corelow.c: avoid repeated warnings in build_file_mappings

Message ID 20230531160406.3932028-3-lancelot.six@amd.com
State New
Headers
Series Fix use-after-free in gdb/corelow.c + cleanups |

Commit Message

Lancelot SIX May 31, 2023, 4:04 p.m. UTC 
  When GDB opens a coredump it tries to locate and then open all files
which were mapped in the process.

If a file is found but cannot be opened with BFD (bfd_open /
bfd_check_format fails), then a warning is printed to the user.  If the
same file was mapped multiple times in the process's address space, the
warning is printed once for each time the file was mapped.  I find this
un-necessarily noisy.

This patch makes it so the warning message is printed only once per
file.

There was a comment in the code assuming that if the file was found on
the system, opening it (bfd_open + bfd_check_format) should always
succeed.  A recent change in BFD (014a602b86f "Don't optimise bfd_seek
to same position") showed that this assumption is not valid.  For
example, it is possible to have a core dump of a process which had
mmaped an IO page from a DRI render node (/dev/dri/runderD$NUM).  In
such case the core dump does contain the information that portions of
this special file were mapped in the host process, but trying to seek to
position 0 will fail, making bfd_check_format fail.  This patch removes
this comment.
---
 gdb/corelow.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Comments

Andrew Burgess June 1, 2023, 9:50 a.m. UTC | #1 
Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:

> When GDB opens a coredump it tries to locate and then open all files
> which were mapped in the process.
>
> If a file is found but cannot be opened with BFD (bfd_open /
> bfd_check_format fails), then a warning is printed to the user.  If the
> same file was mapped multiple times in the process's address space, the
> warning is printed once for each time the file was mapped.  I find this
> un-necessarily noisy.
>
> This patch makes it so the warning message is printed only once per
> file.
>
> There was a comment in the code assuming that if the file was found on
> the system, opening it (bfd_open + bfd_check_format) should always
> succeed.  A recent change in BFD (014a602b86f "Don't optimise bfd_seek
> to same position") showed that this assumption is not valid.  For
> example, it is possible to have a core dump of a process which had
> mmaped an IO page from a DRI render node (/dev/dri/runderD$NUM).  In
> such case the core dump does contain the information that portions of
> this special file were mapped in the host process, but trying to seek to
> position 0 will fail, making bfd_check_format fail.  This patch removes
> this comment.

LGTM.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

> ---
>  gdb/corelow.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 77fc4453f94..ce68f91132e 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -264,18 +264,11 @@ core_target::build_file_mappings ()
>  	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
>  	      {
>  		m_core_unavailable_mappings.emplace_back (start, end - start);
> -		/* If we get here, there's a good chance that it's due to
> -		   an internal error.  We issue a warning instead of an
> -		   internal error because of the possibility that the
> -		   file was removed in between checking for its
> -		   existence during the expansion in exec_file_find()
> -		   and the calls to bfd_openr() / bfd_check_format(). 
> -		   Output both the path from the core file note along
> -		   with its expansion to make debugging this problem
> -		   easier.  */
> -		warning (_("Can't open file %s which was expanded to %s "
> -			   "during file-backed mapping note processing"),
> -			 filename, expanded_fname.get ());
> +		if (unavailable_paths.insert (filename).second)
> +		  warning (_("Can't open file %s which was expanded to %s "
> +			     "during file-backed mapping note processing"),
> +			   filename, expanded_fname.get ());
> +
>  		if (bfd != nullptr)
>  		  {
>  		    bfd_close (bfd);
> -- 
> 2.34.1
diff mbox series

Patch

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 77fc4453f94..ce68f91132e 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -264,18 +264,11 @@  core_target::build_file_mappings ()
 	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
 	      {
 		m_core_unavailable_mappings.emplace_back (start, end - start);
-		/* If we get here, there's a good chance that it's due to
-		   an internal error.  We issue a warning instead of an
-		   internal error because of the possibility that the
-		   file was removed in between checking for its
-		   existence during the expansion in exec_file_find()
-		   and the calls to bfd_openr() / bfd_check_format(). 
-		   Output both the path from the core file note along
-		   with its expansion to make debugging this problem
-		   easier.  */
-		warning (_("Can't open file %s which was expanded to %s "
-			   "during file-backed mapping note processing"),
-			 filename, expanded_fname.get ());
+		if (unavailable_paths.insert (filename).second)
+		  warning (_("Can't open file %s which was expanded to %s "
+			     "during file-backed mapping note processing"),
+			   filename, expanded_fname.get ());
+
 		if (bfd != nullptr)
 		  {
 		    bfd_close (bfd);