Ensure that BIO_eof only returns 1, 0 or a negative value#30395
Ensure that BIO_eof only returns 1, 0 or a negative value#30395nhorman wants to merge 4 commits intoopenssl:masterfrom
Conversation
Recently we uncovered the fact that some platforms (nonstop) return a non-one positive value from feof to indicate end of file. This is in compliance with posix standards, but we had some code that assumed 1 would always be the returned value for an EOF condition, causing various failures. Fix it by converting BIO_eof to only return 0 or 1 to reflect the EOF state (or in the windows case -EINVAL if an invalid stream was passed Fixes openssl#30348
nhorman
requested review from
Sashan,
andrewkdinh,
bob-beck,
esyr,
jogme,
mattcaswell,
npajkovsky and
t8m
nhorman
added
branch: master
| else | ||
| ret = (long)feof(fp); | ||
| ret = !!(long)feof(fp); | ||
| #if defined(OPENSSL_SYS_WINDOWS) |
There was a problem hiding this comment.
Does OPENSSL_SYS_WINDOWS mean MS CRT usage (as opposing to MinGW CRT, for example)?
There was a problem hiding this comment.
I don't know, but ostensibly windows-like platform that don't conform to the windows documented behavior in which feof returns 0/EINVAL on a bad stream pointer will follow the POSIX standard and never set errno, making this a no-op. If there is some other behavior out there, I think we should probably wait to encounter it before we go making assumptions about it.
There was a problem hiding this comment.
My concern with the POSIX behaviour is that, since errno is not touched there, I would assume that it may contain a stale value as a result (and MinGW CRT, being POSIX-conformant, supposedly, to fall under that umbrella).
There was a problem hiding this comment.
its moot. Cygwin on our environment defines OPENSSL_SYS_CYGWIN, rather than OPENSSL_SYS_WINDOWS
|
@nhorman I hope nobody depends on this non-standard feof return values from BIO_eof(). For robustness we should also change the condition in bio_read_intern() from Could you please add that? |
| * Check for that here, and set ret to -EINVAL if its the case. | ||
| */ | ||
| if (ret == 0 && errno == EINVAL) | ||
| ret = -EINVAL; |
There was a problem hiding this comment.
Why -EINVAL? Shouldn't it be just ret = 1; ?
There was a problem hiding this comment.
The docs for BIO_eof state that we return a negative value on error:
https://docs.openssl.org/master/man3/BIO_ctrl/#description
ostensibly? For just this purpose
t8m
added
triaged: bug
Please note there might be 3rd party BIOs that do not return 1 but other non-zero value. Another option would be to change the BIO_eof() implementation to compare with zero. |
According to the docs BIO_eof() return 1, 0 or a negative number - so all it needs to do is convert a return value of > 1 to 1. This seems like a sensible idea (probably in addition to the changes already in this PR) |
openssl-machine
added
approval: done
|
This pull request is ready to merge |
openssl-machine
added
the
approval: ready to merge
|
merged to master and 4.0, thank you |
Recently we uncovered the fact that some platforms (nonstop) return a non-one positive value from feof to indicate end of file. This is in compliance with posix standards, but we had some code that assumed 1 would always be the returned value for an EOF condition, causing various failures. Fix it by converting BIO_eof to only return 0 or 1 to reflect the EOF state (or in the windows case -EINVAL if an invalid stream was passed Fixes #30348 Reviewed-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> MergeDate: Sun Mar 15 19:22:41 2026 (Merged from #30395) (cherry picked from commit 43b03f2)
Recently we uncovered the fact that some platforms (nonstop) return a non-one positive value from feof to indicate end of file. This is in compliance with posix standards, but we had some code that assumed 1 would always be the returned value for an EOF condition, causing various failures. Fix it by converting BIO_eof to only return 0 or 1 to reflect the EOF state (or in the windows case -EINVAL if an invalid stream was passed Fixes #30348 Reviewed-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> MergeDate: Sun Mar 15 19:22:41 2026 (Merged from #30395)
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>
5 participants
Recently we uncovered the fact that some platforms (nonstop) return a non-one positive value from feof to indicate end of file. This is in compliance with posix standards, but we had some code that assumed 1 would always be the returned value for an EOF condition, causing various failures.
Fix it by converting BIO_eof to only return 0 or 1 to reflect the EOF state (or in the windows case -EINVAL if an invalid stream was passed
Fixes #30348