From: Oliver Kurth <okurth@vmware.com>
Date: Fri, 15 Sep 2017 18:23:33 +0000 (-0700)
Subject: Remove NOT_TESTED() from EINTR handling in lib/file
X-Git-Tag: stable-10.2.0~267
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c024d866ac746474487fea646f4787875e2dacdd;p=thirdparty%2Fopen-vm-tools.git

Remove NOT_TESTED() from EINTR handling in lib/file

When FileIO_Read() & friends receive EINTR, they log NOT_TESTED, and
retry read.  It seems innocent - until you provide your own function
to handle logging.

Now imagine that you create bidirectional communication protocol,
and run it over the pipe.  And you do logging over this very same
pipe.  Application is waiting for message, so it invoked
FileIO_Read(), and is blocked on read().  Now you attach strace
to the process.  That will interrupt pending read() with EINTR
and log NOT_TESTED().  This logging performs FileIO_Write() on
this very same pipe, and then performs FileIO_Read() to retrieve
status of logging request.

If it receives status of logging request, all is good.  Code
returns from logging, and reexecutes read(), waiting for
request from server.

But if logging function receives anything else (f.e. other side
sends request at same time strace was attached), it gets queued
into internal data structures of the app for processing once
code returns to main application loop.

Problem is that FileIO_Read() will reenter read() without
returning to the caller.  And that read() will block forever:
other end already send request that is now pending in the
request list, and so won't send anything until it sees
we processed that pending request.  Hang.

There are two possible fixes:

1. Return EINTR from FileIO_Read(), or

2. Do not do any logging from FileIO_Read()

Approach #2 is much easier, as API semantic does not change,
and currently it is broken.  So there is no need for NOT_TESTED()
anymore...
---

diff --git a/open-vm-tools/lib/file/fileIOPosix.c b/open-vm-tools/lib/file/fileIOPosix.c
index 73f7888fc..ee9f4b002 100644
--- a/open-vm-tools/lib/file/fileIOPosix.c
+++ b/open-vm-tools/lib/file/fileIOPosix.c
@@ -1255,7 +1255,6 @@ FileIO_Write(FileIODescriptor *fd,  // IN:
          int error = errno;
 
          if (error == EINTR) {
-            NOT_TESTED();
             continue;
          }
          fret = FileIOErrno2Result(error);
@@ -1315,7 +1314,6 @@ FileIO_Read(FileIODescriptor *fd,  // IN:
       res = read(fd->posix, buf, requested);
       if (res == -1) {
          if (errno == EINTR) {
-            NOT_TESTED();
             continue;
          }
          fret = FileIOErrno2Result(errno);
@@ -1613,7 +1611,6 @@ FileIO_Readv(FileIODescriptor *fd,  // IN:
 
       if (retval == -1) {
          if (errno == EINTR) {
-            NOT_TESTED();
             continue;
          }
          fret = FileIOErrno2Result(errno);
@@ -1726,7 +1723,6 @@ FileIO_Writev(FileIODescriptor *fd,  // IN:
 
       if (retval == -1) {
          if (errno == EINTR) {
-            NOT_TESTED();
             continue;
          }
          fret = FileIOErrno2Result(errno);
@@ -1822,8 +1818,6 @@ FileIOPreadvCoalesced(
 
          if (retval == -1) {
             if (errno == EINTR) {
-               LOG_ONCE((LGPFX" %s got EINTR.  Retrying\n", __FUNCTION__));
-               NOT_TESTED_ONCE();
                continue;
             }
             fret = FileIOErrno2Result(errno);
@@ -1911,8 +1905,6 @@ FileIOPwritevCoalesced(
 
          if (retval == -1) {
             if (errno == EINTR) {
-               LOG_ONCE((LGPFX" %s got EINTR.  Retrying\n", __FUNCTION__));
-               NOT_TESTED_ONCE();
                continue;
             }
             fret = FileIOErrno2Result(errno);
@@ -2028,7 +2020,6 @@ FileIOPreadvInternal(
       }
       if (retval == -1) {
          if (errno == EINTR) {
-            NOT_TESTED();
             continue;
          }
          if (errno == ENOSYS || errno == EINVAL || errno == ENOMEM) {
@@ -2164,7 +2155,6 @@ FileIOPwritevInternal(
       }
       if (retval == -1) {
          if (errno == EINTR) {
-            NOT_TESTED();
             continue;
          }
          if (errno == ENOSYS || errno == EINVAL || errno == ENOMEM) {