Opened 3 years ago

Closed 3 years ago

#20988 closed defect (fixed)

Test fgets_eagain fails on FreeBSD-amd64

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: freebsd, tor-03-unspecified-201612
Cc: ahf Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

According to the BSD Buildbot

util/fgets_eagain: 
  FAIL src/test/test_util.c:3952: assert(retptr OP_EQ buf): 0x0 vs 0x7fffffffe944
  [fgets_eagain FAILED]

This means that fgets returns a null pointer on partial lines instead of the buffer as expected.

Previously this test was passing but started failing with build #105. Looking at the changes to libc it looks like this is caused by revision 305413 (which was added earlier in the same month the test started failing).

I'm unsure whether FreeBSD is right and other libc implementations are wrong or the other way around.

Child Tickets

Change History (12)

comment:1 Changed 3 years ago by nickm

Keywords: freebsd added
Milestone: Tor: 0.3.???

comment:2 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:3 Changed 3 years ago by cypherpunks

Closed #21416 as a duplicate.

comment:4 Changed 3 years ago by ahf

Keywords: review-group-16 added

I've added a patch for this in https://gitlab.com/ahf/tor/commits/bugs/20988

comment:5 Changed 3 years ago by nickm

Keywords: review-group-16 removed
Milestone: Tor: unspecifiedTor: 0.3.1.x-final
Status: newneeds_review

comment:6 in reply to:  4 Changed 3 years ago by cypherpunks

Replying to ahf:

I've added a patch for this in https://gitlab.com/ahf/tor/commits/bugs/20988

OP here. The changes look good but i haven't tested them. The inline patch fixes some small grammar nitpicks on commit 49a4069d19877b24f00220fb41410445c804b463.

diff --git a/src/common/compat.c b/src/common/compat.c
index 771cc085d..753ad3f8f 100644
--- a/src/common/compat.c
+++ b/src/common/compat.c
@@ -3480,7 +3480,7 @@ tor_getpass(const char *prompt, char *output, size_t buflen)
  * Upon successful completion, this function returns a pointer to the string
  * <b>str</b>. If EOF occurs before any characters are read the function will
  * return NULL and the content of <b>str</b> is unchanged. Upon error, the
- * function return NULL and the caller must check for error using foef(3) and
+ * function returns NULL and the caller must check for error using foef(3) and
  * ferror(3).
  */
 char *
@@ -3490,10 +3490,10 @@ tor_fgets(char *str, int size, FILE *stream)
 
   ret = fgets(str, size, stream);
 
-  /* FreeBSD, OpenBSD, Linux (glibc), and Linux (musl) seems to disagree about
+  /* FreeBSD, OpenBSD, Linux (glibc), and Linux (musl) seem to disagree about
    * what to do in the given situation. We check if the stream has been flagged
-   * with an error-bit and retur NULL in that situation if errno is also set to
-   * EAGAIN.
+   * with an error-bit and return NULL in that situation if errno is also set
+   * to EAGAIN.
    */
   if (ferror(stream) && errno == EAGAIN)
     return NULL;

comment:7 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

Looks good -- needs a changes file though? Also, is this tested?

comment:8 Changed 3 years ago by ahf

Cc: ahf added
Status: needs_revisionneeds_review

I've updated my branch with the following changes:

  • Fixed typos reported by OP. Thanks!
  • Added a changes file.
  • We now clear the error flag on the file handler in the case where ferror(stream) && errno == EAGAIN to prevent the situation where the caller receives an error return value and retries because of errno being EAGAIN, but continues to have the error flag set.

I ran make check and tested it with "normal" browsing usage on OS X, HardenedBSD, and Linux.

comment:9 Changed 3 years ago by ahf

Status: needs_reviewneeds_revision

The clearerr() call is incorrect. Let me upload a revised version.

comment:10 Changed 3 years ago by ahf

Status: needs_revisionneeds_review

Ignore the third bullet in comment 8: Instead of clearing the error flag of the file handle we now reset the errno variable before we call fgets(3).

comment:11 Changed 3 years ago by nickm

Merged this. Added #21654 for a more elegant solution.

comment:12 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.