Tolerate feof returning arbitrary non-zero values

feof apparently does not promise EOF returns 1, just a non-zero value.
See [0]. We very much do not care about the impacted platform, but it is
probably prudent to handle this, in case some platform we do care about
uses this leeway. While I'm here, add a test for EOF in file BIOs.

In writing this test, I noticed that BIO_reset's return value
convention, like BIO_seek, is completely inconsistent. Document the
problem for now. (OpenSSL's documentation[1] also mentions this, though
they seem to have missed that fds are also impacted.)

[0] openssl/openssl#30395
[1] https://docs.openssl.org/master/man3/BIO_ctrl/#description:~:text=%C2%B6-,BIO_reset()%20normally%20returns%201%20for%20success%20and%20%3C%3D0%20for%20failure.%20File%20BIOs%20are%20an%20exception%2C%20they%20return%200%20for%20success%20and%20%2D1%20for%20failure.,-BIO_seek()%20and%20BIO_tell

Change-Id: I2df1925b157bb34897174143d74ca9e5e86a673c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/90888
Reviewed-by: Lily Chen <chlily@google.com>
Commit-Queue: Lily Chen <chlily@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

Author: davidben

crypto/bio/bio_test.cc

@@ -704,6 +704,37 @@ TEST(BIOTest, Gets) {
   EXPECT_EQ(c, 'a');
 }
 
+TEST(BIOTest, FileEOF) {
+  if (SkipTempFileTests()) {
+    GTEST_SKIP();
+  }
+
+  TemporaryFile file;
+  ASSERT_TRUE(file.Init("abcd"));
+  UniquePtr<BIO> bio(BIO_new_file(file.path().c_str(), "rb"));
+  ASSERT_TRUE(bio);
+  EXPECT_EQ(BIO_eof(bio.get()), 0);
+  // Read everything.
+  char buf[4];
+  ASSERT_EQ(4, BIO_read(bio.get(), buf, sizeof(buf)));
+  EXPECT_EQ(Bytes(buf, sizeof(buf)), Bytes("abcd"));
+  EXPECT_EQ(BIO_eof(bio.get()), 0);
+  // Try to keep reading. The BIO should now signal EOF.
+  ASSERT_EQ(0, BIO_read(bio.get(), buf, sizeof(buf)));
+  EXPECT_EQ(BIO_eof(bio.get()), 1);
+  // Reset the BIO. File BIOs have an unusual return value convention for
+  // |BIO_reset|.
+  EXPECT_EQ(BIO_reset(bio.get()), 0);
+  // This should clear the EOF indicator for the stream.
+  EXPECT_EQ(BIO_eof(bio.get()), 0);
+  // Repeat the test.
+  ASSERT_EQ(4, BIO_read(bio.get(), buf, sizeof(buf)));
+  EXPECT_EQ(Bytes(buf, sizeof(buf)), Bytes("abcd"));
+  EXPECT_EQ(BIO_eof(bio.get()), 0);
+  ASSERT_EQ(0, BIO_read(bio.get(), buf, sizeof(buf)));
+  EXPECT_EQ(BIO_eof(bio.get()), 1);
+}
+
 // Test that, on Windows, file BIOs correctly handle text vs binary mode.
 TEST(BIOTest, FileMode) {
   if (SkipTempFileTests()) {

crypto/bio/file.cc

@@ -147,7 +147,8 @@ static long file_ctrl(BIO *b, int cmd, long num, void *ptr) {
     case BIO_C_FILE_SEEK:
       return fseek(fp, num, 0);
     case BIO_CTRL_EOF:
-      return feof(fp);
+      // feof may return any non-zero value for EOF, but we must return 1.
+      return feof(fp) != 0;
     case BIO_C_FILE_TELL:
     case BIO_CTRL_INFO:
       return ftell(fp);

include/openssl/bio.h

@@ -118,8 +118,12 @@ OPENSSL_EXPORT char *BIO_ptr_ctrl(BIO *bp, int cmd, long larg);
 OPENSSL_EXPORT long BIO_int_ctrl(BIO *bp, int cmd, long larg, int iarg);
 
 // BIO_reset resets |bio| to its initial state, the precise meaning of which
-// depends on the concrete type of |bio|. It returns one on success and zero
-// otherwise.
+// depends on the concrete type of |bio|. It normally returns one on success and
+// <= 0 otherwise. However, for file and fd BIOs, it returns zero on success and
+// a negative number on error.
+//
+// WARNING: This function's return value conventions differs from most functions
+// in this library.
 OPENSSL_EXPORT int BIO_reset(BIO *bio);
 
 // BIO_eof returns non-zero when |bio| has reached end-of-file. The precise
@@ -496,8 +500,8 @@ OPENSSL_EXPORT int BIO_rw_filename(BIO *bio, const char *filename);
 // BIO_tell returns the file offset of |bio|, or a negative number on error or
 // if |bio| does not support the operation.
 //
-// TODO(https://crbug.com/boringssl/465): On platforms where |long| is 32-bit,
-// this function cannot report 64-bit offsets.
+// TODO(crbug.com/42290329): On platforms where |long| is 32-bit, this function
+// cannot report 64-bit offsets.
 OPENSSL_EXPORT long BIO_tell(BIO *bio);
 
 // BIO_seek sets the file offset of |bio| to |offset|. It returns a non-negative
@@ -508,8 +512,8 @@ OPENSSL_EXPORT long BIO_tell(BIO *bio);
 // WARNING: This function's return value conventions differs from most functions
 // in this library.
 //
-// TODO(https://crbug.com/boringssl/465): On platforms where |long| is 32-bit,
-// this function cannot handle 64-bit offsets.
+// TODO(crbug.com/42290329): On platforms where |long| is 32-bit, this function
+// cannot handle 64-bit offsets.
 OPENSSL_EXPORT long BIO_seek(BIO *bio, long offset);