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

[Andrew Burgess <aburgess@redhat.com>]

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?

I'm not arguing with your reading of the spec, but that does seem
unlikely.  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.

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

Thanks,
Andrew