Re: [PATCH v2] [gdb] Handle EINTR in fgets

[Tom de Vries <tdevries@suse.de>]

On 8/18/25 12:23, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> Usually, find_charset_names calls iconv -l to get the list of supported
>> charsets, allowing us to use a charset not in the default list:
>> ...
>> $ /usr/bin/gdb -q -batch -ex "set charset CP858"
>> $
>> ...
>>
>> If the call to iconv -l fails somehow, gdb silently falls back to using the
>> default list, which gets us instead:
>> ...
>> $ PATH= /usr/bin/gdb -q -batch -ex "set charset CP858"
>> Undefined item: "CP858".
>> $
>> ...
>>
>> PR gdb/33274 reports that gdb occasionally fails in the same way, because
>> fgets returns nullptr before the output of iconv -l is read entirely.
>>
>> We asked the reporter to try out a patch handling errno == EINTR after
>> fgets returns nullptr, and that fixed the problem.
>>
>> Fix this by:
>> - adding an inline function gdb::fgets that handles EINTR, and
>> - using it instead of fgets.
>>
>> Tested on x86_64-linux.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33274
>> ---
>>   gdb/charset.c               |  3 ++-
>>   gdb/linux-nat.c             |  3 ++-
>>   gdb/nat/linux-btrace.c      |  3 ++-
>>   gdb/nat/linux-osdata.c      | 13 +++++++------
>>   gdb/nat/linux-procfs.c      |  7 ++++---
>>   gdbserver/linux-i386-ipa.cc |  3 ++-
>>   gdbsupport/eintr.h          | 15 +++++++++++++++
>>   7 files changed, 34 insertions(+), 13 deletions(-)
>>
>> diff --git a/gdb/charset.c b/gdb/charset.c
>> index 259362563b2..abf3fe800d2 100644
>> --- a/gdb/charset.c
>> +++ b/gdb/charset.c
>> @@ -25,6 +25,7 @@
>>   #include "gdbsupport/environ.h"
>>   #include "arch-utils.h"
>>   #include <ctype.h>
>> +#include "gdbsupport/eintr.h"
>>   
>>   #ifdef USE_WIN32API
>>   #include <windows.h>
>> @@ -842,7 +843,7 @@ find_charset_names (void)
>>   	  char *start, *r;
>>   	  int len;
>>   
>> -	  r = fgets (buf, sizeof (buf), in);
>> +	  r = gdb::fgets (buf, sizeof (buf), in);
>>   	  if (!r)
>>   	    break;
>>   	  len = strlen (r);
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index f3179279d92..39f1fb59ae0 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -64,6 +64,7 @@
>>   #include "gdbsupport/scope-exit.h"
>>   #include "gdbsupport/gdb-sigmask.h"
>>   #include "gdbsupport/common-debug.h"
>> +#include "gdbsupport/eintr.h"
>>   #include <unordered_map>
>>   
>>   /* This comment documents high-level logic of this file.
>> @@ -4301,7 +4302,7 @@ linux_proc_pending_signals (int pid, sigset_t *pending,
>>     if (procfile == NULL)
>>       error (_("Could not open %s"), fname);
>>   
>> -  while (fgets (buffer, PATH_MAX, procfile.get ()) != NULL)
>> +  while (gdb::fgets (buffer, PATH_MAX, procfile.get ()) != NULL)
>>       {
>>         /* Normal queued signals are on the SigPnd line in the status
>>   	 file.  However, 2.6 kernels also have a "shared" pending
>> diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
>> index 9eadd46c9be..08c7b795a86 100644
>> --- a/gdb/nat/linux-btrace.c
>> +++ b/gdb/nat/linux-btrace.c
>> @@ -26,6 +26,7 @@
>>   #include "gdbsupport/filestuff.h"
>>   #include "gdbsupport/scoped_fd.h"
>>   #include "gdbsupport/scoped_mmap.h"
>> +#include "gdbsupport/eintr.h"
>>   
>>   #include <inttypes.h>
>>   
>> @@ -202,7 +203,7 @@ linux_determine_kernel_start (void)
>>         uint64_t addr;
>>         int match;
>>   
>> -      line = fgets (buffer, sizeof (buffer), file.get ());
>> +      line = gdb::fgets (buffer, sizeof (buffer), file.get ());
>>         if (line == NULL)
>>   	break;
>>   
>> diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
>> index b52a8ed5f36..bfdc26f2795 100644
>> --- a/gdb/nat/linux-osdata.c
>> +++ b/gdb/nat/linux-osdata.c
>> @@ -35,6 +35,7 @@
>>   #include <dirent.h>
>>   #include <sys/stat.h>
>>   #include "gdbsupport/filestuff.h"
>> +#include "gdbsupport/eintr.h"
>>   #include <algorithm>
>>   #include "linux-procfs.h"
>>   
>> @@ -562,7 +563,7 @@ linux_xfer_osdata_cpus ()
>>   
>>         do
>>   	{
>> -	  if (fgets (buf, sizeof (buf), fp.get ()))
>> +	  if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>>   	    {
>>   	      char *key, *value;
>>   	      int i = 0;
>> @@ -780,7 +781,7 @@ print_sockets (unsigned short family, int tcp, std::string &buffer)
>>   
>>         do
>>   	{
>> -	  if (fgets (buf, sizeof (buf), fp.get ()))
>> +	  if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>>   	    {
>>   	      uid_t uid;
>>   	      unsigned int local_port, remote_port, state;
>> @@ -961,7 +962,7 @@ linux_xfer_osdata_shm ()
>>   
>>         do
>>   	{
>> -	  if (fgets (buf, sizeof (buf), fp.get ()))
>> +	  if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>>   	    {
>>   	      key_t key;
>>   	      uid_t uid, cuid;
>> @@ -1057,7 +1058,7 @@ linux_xfer_osdata_sem ()
>>   
>>         do
>>   	{
>> -	  if (fgets (buf, sizeof (buf), fp.get ()))
>> +	  if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>>   	    {
>>   	      key_t key;
>>   	      uid_t uid, cuid;
>> @@ -1137,7 +1138,7 @@ linux_xfer_osdata_msg ()
>>   
>>         do
>>   	{
>> -	  if (fgets (buf, sizeof (buf), fp.get ()))
>> +	  if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>>   	    {
>>   	      key_t key;
>>   	      PID_T lspid, lrpid;
>> @@ -1231,7 +1232,7 @@ linux_xfer_osdata_modules ()
>>   
>>         do
>>   	{
>> -	  if (fgets (buf, sizeof (buf), fp.get ()))
>> +	  if (gdb::fgets (buf, sizeof (buf), fp.get ()))
>>   	    {
>>   	      char *name, *dependencies, *status, *tmp, *saveptr;
>>   	      unsigned int size;
>> diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
>> index d4f9af32bb9..1927a21314e 100644
>> --- a/gdb/nat/linux-procfs.c
>> +++ b/gdb/nat/linux-procfs.c
>> @@ -19,6 +19,7 @@
>>   #include "linux-procfs.h"
>>   #include "gdbsupport/filestuff.h"
>>   #include "gdbsupport/unordered_set.h"
>> +#include "gdbsupport/eintr.h"
>>   #include <dirent.h>
>>   #include <sys/stat.h>
>>   #include <utility>
>> @@ -42,7 +43,7 @@ linux_proc_get_int (pid_t lwpid, const char *field, int warn)
>>         return -1;
>>       }
>>   
>> -  while (fgets (buf, sizeof (buf), status_file.get ()))
>> +  while (gdb::fgets (buf, sizeof (buf), status_file.get ()))
>>       if (strncmp (buf, field, field_len) == 0 && buf[field_len] == ':')
>>         {
>>   	retval = strtol (&buf[field_len + 1], NULL, 10);
>> @@ -140,7 +141,7 @@ linux_proc_pid_get_state (pid_t pid, int warn, enum proc_state *state)
>>       }
>>   
>>     have_state = 0;
>> -  while (fgets (buffer, sizeof (buffer), procfile.get ()) != NULL)
>> +  while (gdb::fgets (buffer, sizeof (buffer), procfile.get ()) != NULL)
>>       if (startswith (buffer, "State:"))
>>         {
>>   	have_state = 1;
>> @@ -317,7 +318,7 @@ linux_proc_tid_get_name (ptid_t ptid)
>>     if (comm_file == NULL)
>>       return NULL;
>>   
>> -  comm_val = fgets (comm_buf, sizeof (comm_buf), comm_file.get ());
>> +  comm_val = gdb::fgets (comm_buf, sizeof (comm_buf), comm_file.get ());
>>   
>>     if (comm_val != NULL)
>>       {
>> diff --git a/gdbserver/linux-i386-ipa.cc b/gdbserver/linux-i386-ipa.cc
>> index 17af6eb3610..7d6b9600c34 100644
>> --- a/gdbserver/linux-i386-ipa.cc
>> +++ b/gdbserver/linux-i386-ipa.cc
>> @@ -21,6 +21,7 @@
>>   #include <sys/mman.h>
>>   #include "tracepoint.h"
>>   #include "gdbsupport/x86-xstate.h"
>> +#include "gdbsupport/eintr.h"
>>   #include "arch/i386-linux-tdesc.h"
>>   #include "arch/x86-linux-tdesc-features.h"
>>   
>> @@ -138,7 +139,7 @@ initialize_fast_tracepoint_trampoline_buffer (void)
>>         return;
>>       }
>>   
>> -  if (fgets (buf, IPA_BUFSIZ, f))
>> +  if (gdb::fgets (buf, IPA_BUFSIZ, f))
>>       sscanf (buf, "%llu", &mmap_min_addr);
>>         
>>     fclose (f);
>> diff --git a/gdbsupport/eintr.h b/gdbsupport/eintr.h
>> index 04f86c8c3df..800cab561b4 100644
>> --- a/gdbsupport/eintr.h
>> +++ b/gdbsupport/eintr.h
>> @@ -116,6 +116,21 @@ write (int fd, const void *buf, size_t count)
>>     return gdb::handle_eintr (-1, ::write, fd, buf, count);
>>   }
>>   
>> +inline char*
>> +fgets (char *str, int count, FILE* stream)
>> +{
>> +  char *ret;
>> +
>> +  do
>> +    {
>> +      errno = 0;
>> +      ret = ::fgets (str, count, stream);
>> +    }
>> +  while (ret == nullptr && ferror (stream) && errno == EINTR);
> 
> Having read through the v1 thread, I just want to make sure I'm clear on
> the motivation for not using handle_eintr:  your concern is that errno
> might be set in the EOF case?
> 

Hi Andrew,

thanks for the review.

Yes, that is my concern.

> I'm not arguing with your reading of the spec, but that does seem
> unlikely. 

Agreed.

> So much so that I do think that's worth a comment explaining
> the logic here.  It's also worth the comment explaining that we've never
> _seen_ such behaviour, just that we're worried such behaviour _might_ be
> in-spec.
> 

Good point.  I've added a comment in a v3 ( 
https://sourceware.org/pipermail/gdb-patches/2025-August/220017.html ).

> That comment could fill in the missing header comment for this function.
> 

Since it's a comment about the implementation of the function, I've 
added it in the function.

[ The missing header comment seems to be a feature of all the functions 
in the file, probably because they're wrappers and the interface is 
defined by what they're wrapping, and there's only one difference in 
behaviour implemented by the wrapper, which is described in detail at 
the top of the file. ]

Thanks,
- Tom

> Thanks,
> Andrew
>