[libc] fwrite_unlocked: only return errno if an actual error occurred. #167350
Conversation
fwrite and friends don't modify errno if no error occurred. Therefore frite_unlocked's return value shouldn't be constructed from errno without checking if an error actually occurred.
|
@llvm/pr-subscribers-libc Author: None (Sterling-Augustine) Changesfwrite and friends don't modify errno if no error occurred. Therefore frite_unlocked's return value shouldn't be constructed from errno without checking if an error actually occurred. Full diff: https://github.com/llvm/llvm-project/pull/167350.diff 1 Files Affected:
diff --git a/libc/src/stdio/printf_core/vfprintf_internal.h b/libc/src/stdio/printf_core/vfprintf_internal.h
index 564441d3bf51a..a14ddee5f62fd 100644
--- a/libc/src/stdio/printf_core/vfprintf_internal.h
+++ b/libc/src/stdio/printf_core/vfprintf_internal.h
@@ -51,8 +51,14 @@ LIBC_INLINE void funlockfile(::FILE *f) { ::funlockfile(f); }
LIBC_INLINE FileIOResult fwrite_unlocked(const void *ptr, size_t size,
size_t nmemb, ::FILE *f) {
// Need to use system errno in this case, as system write will set this errno
- // which we need to propagate back into our code.
- return {::fwrite_unlocked(ptr, size, nmemb, f), errno};
+ // which we need to propagate back into our code. fwrite only modifies errno
+ // if there was an error, and errno may have previously been nonzero. Only
+ // return errno if there was an error.
+ auto bytes = ::fwrite_unlocked(ptr, size, nmemb, f);
+ if (bytes == nmemb * size)
+ return bytes;
+ else
+ return errno;
}
#endif // LIBC_COPT_STDIO_USE_SYSTEM_FILE
} // namespace internal
|
| if (bytes == nmemb * size) | ||
| return bytes; | ||
| else | ||
| return errno; |
There was a problem hiding this comment.
I don't think you can construct FileIOResult like that? Shouldn't you return smth. like {bytes, errno} here? Is there a way to add a test that if system errno was pre-set, but the call succeeded, this call wouldn't return system errno?
michaelrj-google
left a comment
michaelrj-google
left a comment
There was a problem hiding this comment.
LGTM, I forgot about system errno potentially being already set from an unrelated call
| // if there was an error, and errno may have previously been nonzero. Only | ||
| // return errno if there was an error. | ||
| auto bytes = ::fwrite_unlocked(ptr, size, nmemb, f); | ||
| return {bytes, bytes == nmemb * size ? 0 : errno}; |
There was a problem hiding this comment.
Docs for fwrite say say that it returns the number of elements written, not bytes (https://man7.org/linux/man-pages/man3/fwrite.3p.html#RETURN_VALUE). Is fwrite_unlocked different?
There was a problem hiding this comment.
good catch fixed.
| // if there was an error, and errno may have previously been nonzero. Only | ||
| // return errno if there was an error. | ||
| size_t bytes = ::fwrite_unlocked(ptr, size, nmemb, f); | ||
| return {bytes, bytes == size ? 0 : errno}; |
There was a problem hiding this comment.
The fwrite() function shall return the number of elements successfully written, which may be less than
nitemsif a write error is encountered.
https://man7.org/linux/man-pages/man3/fwrite.3p.html#RETURN_VALUE
I think that instead of bytes == size ? 0, it should be bytes == nmemb ? 0. Could we also update the name bytes to reflect that it's the number of elements rather than number of bytes?
There was a problem hiding this comment.
cleaned up.
5 participants
fwrite and friends don't modify errno if no error occurred. Therefore frite_unlocked's return value shouldn't be constructed from errno without checking if an error actually occurred.
This fixes an error introduced by 9e2f73f