* [PATCH] gdb: Fix "target file /proc/.../cmdline contained unexpected null characters"
@ 2023-06-21 23:14 Ilya Leoshkevich
2023-07-18 13:03 ` Bruno Larsen
0 siblings, 1 reply; 3+ messages in thread
From: Ilya Leoshkevich @ 2023-06-21 23:14 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches, Ilya Leoshkevich
cmdline is read with target_fileio_read_stralloc(), which warns on
seeing null characters. However, it's perfectly valid for cmdline to
contain \0s, so switch to target_fileio_read_alloc().
---
gdb/linux-tdep.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index b5eee5e108c..96cbe8e5520 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1902,15 +1902,22 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
pid = inferior_ptid.pid ();
xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", (int) pid);
/* The full name of the program which generated the corefile. */
- gdb::unique_xmalloc_ptr<char> fname
- = target_fileio_read_stralloc (NULL, filename);
+ gdb_byte *buf = NULL;
+ size_t buf_len = target_fileio_read_alloc (NULL, filename, &buf);
+ gdb::unique_xmalloc_ptr<char> fname ((char *)buf);
- if (fname == NULL || fname.get ()[0] == '\0')
+ if (buf_len < 1 || fname.get ()[0] == '\0')
{
/* No program name was read, so we won't be able to retrieve more
information about the process. */
return 0;
}
+ if (fname.get ()[buf_len - 1] != '\0')
+ {
+ warning (_("target file %s "
+ "does not contain a trailing null character"),
+ filename);
+ }
memset (p, 0, sizeof (*p));
--
2.40.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] gdb: Fix "target file /proc/.../cmdline contained unexpected null characters"
2023-06-21 23:14 [PATCH] gdb: Fix "target file /proc/.../cmdline contained unexpected null characters" Ilya Leoshkevich
@ 2023-07-18 13:03 ` Bruno Larsen
2023-07-19 11:00 ` Ilya Leoshkevich
0 siblings, 1 reply; 3+ messages in thread
From: Bruno Larsen @ 2023-07-18 13:03 UTC (permalink / raw)
To: Ilya Leoshkevich, Tom Tromey; +Cc: gdb-patches
On 22/06/2023 01:14, Ilya Leoshkevich via Gdb-patches wrote:
> cmdline is read with target_fileio_read_stralloc(), which warns on
> seeing null characters. However, it's perfectly valid for cmdline to
> contain \0s, so switch to target_fileio_read_alloc().
Hi! Thanks for working on this.
From what I understand, GDB commit messages should be written so that
they make sense even if you don't read the title, so having a slightly
more descriptive message would be nice.
> ---
> gdb/linux-tdep.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index b5eee5e108c..96cbe8e5520 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -1902,15 +1902,22 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
> pid = inferior_ptid.pid ();
> xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", (int) pid);
> /* The full name of the program which generated the corefile. */
> - gdb::unique_xmalloc_ptr<char> fname
> - = target_fileio_read_stralloc (NULL, filename);
> + gdb_byte *buf = NULL;
> + size_t buf_len = target_fileio_read_alloc (NULL, filename, &buf);
> + gdb::unique_xmalloc_ptr<char> fname ((char *)buf);
>
Looking at the code on linux_info_proc, I see that this approach is
already used, so it looks like a good idea to me. However, in there, the
string is slightly sanitized, so that '\0' in the middle of the string
are turned into ' ', and the final character is always set to '\0'. I
think you could probably do the same here.
> - if (fname == NULL || fname.get ()[0] == '\0')
> + if (buf_len < 1 || fname.get ()[0] == '\0')
> {
> /* No program name was read, so we won't be able to retrieve more
> information about the process. */
> return 0;
> }
> + if (fname.get ()[buf_len - 1] != '\0')
If you don't do the same, at least here the last character should be
changed. Its pretty dangerous to allow it to pass since like 5 lines
later this is used as a C-string, so you could get read-past-the-end and
other nasty problems.
--
Cheers,
Bruno
> + {
> + warning (_("target file %s "
> + "does not contain a trailing null character"),
> + filename);
> + }
>
> memset (p, 0, sizeof (*p));
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] gdb: Fix "target file /proc/.../cmdline contained unexpected null characters"
2023-07-18 13:03 ` Bruno Larsen
@ 2023-07-19 11:00 ` Ilya Leoshkevich
0 siblings, 0 replies; 3+ messages in thread
From: Ilya Leoshkevich @ 2023-07-19 11:00 UTC (permalink / raw)
To: Bruno Larsen, Tom Tromey; +Cc: gdb-patches
On Tue, 2023-07-18 at 15:03 +0200, Bruno Larsen wrote:
> On 22/06/2023 01:14, Ilya Leoshkevich via Gdb-patches wrote:
> > cmdline is read with target_fileio_read_stralloc(), which warns on
> > seeing null characters. However, it's perfectly valid for cmdline
> > to
> > contain \0s, so switch to target_fileio_read_alloc().
>
> Hi! Thanks for working on this.
Thanks for the review!
>
> From what I understand, GDB commit messages should be written so
> that
> they make sense even if you don't read the title, so having a
> slightly
> more descriptive message would be nice.
Will do.
> > ---
> > gdb/linux-tdep.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> > index b5eee5e108c..96cbe8e5520 100644
> > --- a/gdb/linux-tdep.c
> > +++ b/gdb/linux-tdep.c
> > @@ -1902,15 +1902,22 @@ linux_fill_prpsinfo (struct
> > elf_internal_linux_prpsinfo *p)
> > pid = inferior_ptid.pid ();
> > xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline",
> > (int) pid);
> > /* The full name of the program which generated the corefile.
> > */
> > - gdb::unique_xmalloc_ptr<char> fname
> > - = target_fileio_read_stralloc (NULL, filename);
> > + gdb_byte *buf = NULL;
> > + size_t buf_len = target_fileio_read_alloc (NULL, filename,
> > &buf);
> > + gdb::unique_xmalloc_ptr<char> fname ((char *)buf);
> >
>
> Looking at the code on linux_info_proc, I see that this approach is
> already used, so it looks like a good idea to me. However, in there,
> the
> string is slightly sanitized, so that '\0' in the middle of the
> string
> are turned into ' ', and the final character is always set to '\0'. I
> think you could probably do the same here.
Here we are interested only in the filename, so ending the C string
at the first '\0' is what we need.
> > - if (fname == NULL || fname.get ()[0] == '\0')
> > + if (buf_len < 1 || fname.get ()[0] == '\0')
> > {
> > /* No program name was read, so we won't be able to
> > retrieve more
> > information about the process. */
> > return 0;
> > }
> > + if (fname.get ()[buf_len - 1] != '\0')
> If you don't do the same, at least here the last character should be
> changed. Its pretty dangerous to allow it to pass since like 5 lines
> later this is used as a C-string, so you could get read-past-the-end
> and
> other nasty problems.
Oh, right - I think I should just return here. /proc/.../cmdline not
having a trailing '\0' must be a kernel bug anyway.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-19 11:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-21 23:14 [PATCH] gdb: Fix "target file /proc/.../cmdline contained unexpected null characters" Ilya Leoshkevich
2023-07-18 13:03 ` Bruno Larsen
2023-07-19 11:00 ` Ilya Leoshkevich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).