[libc] fwrite_unlocked: only return errno if an actual error occurred.

[Sterling-Augustine]

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

[llvmbot]

@llvm/pr-subscribers-libc

Author: None (Sterling-Augustine)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/167350.diff

1 Files Affected:

• (modified) libc/src/stdio/printf_core/vfprintf_internal.h (+8-2)

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

[vonosmas]

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?

[Sterling-Augustine]

done

[michaelrj-google]

LGTM, I forgot about system errno potentially being already set from an unrelated call

[jtstogel]

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?

[Sterling-Augustine]

good catch fixed.

[jtstogel]

The fwrite() function shall return the number of elements successfully written, which may be less than nitems if 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?

[Sterling-Augustine]

cleaned up.