-
Notifications
You must be signed in to change notification settings - Fork 14.9k
release/21.x: [libc++] Ensure that we restore invariants in basic_filebuf::overflow (#147389) #155712
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/21.x
Are you sure you want to change the base?
Conversation
…llvm#147389) In rare circumstances, the invariants could fail to be restored. (cherry picked from commit 6291b63)
@philnik777 What do you think about merging this PR to the release branch? |
@llvm/pr-subscribers-libcxx Author: None (llvmbot) ChangesBackport 6291b63 Requested by: @var-const Full diff: https://github.com/llvm/llvm-project/pull/155712.diff 2 Files Affected:
diff --git a/libcxx/include/fstream b/libcxx/include/fstream
index c86f709bedb80..dc5c47304f014 100644
--- a/libcxx/include/fstream
+++ b/libcxx/include/fstream
@@ -821,6 +821,14 @@ typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>
template <class _CharT, class _Traits>
typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>::overflow(int_type __c) {
+ auto __failed = [this]() {
+ if (this->pptr() == this->epptr() + 1) {
+ this->pbump(-1); // lose the character we overflowed above -- we don't really have a
+ // choice since we couldn't commit the contents of the put area
+ }
+ return traits_type::eof();
+ };
+
if (__file_ == nullptr)
return traits_type::eof();
__write_mode();
@@ -841,8 +849,9 @@ typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>
if (__always_noconv_) {
size_t __n = static_cast<size_t>(this->pptr() - this->pbase());
- if (std::fwrite(this->pbase(), sizeof(char_type), __n, __file_) != __n)
- return traits_type::eof();
+ if (std::fwrite(this->pbase(), sizeof(char_type), __n, __file_) != __n) {
+ return __failed();
+ }
} else {
if (!__cv_)
std::__throw_bad_cast();
@@ -854,34 +863,38 @@ typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>
char* __extbuf_end = __extbuf_;
do {
codecvt_base::result __r = __cv_->out(__st_, __b, __p, __end, __extbuf_, __extbuf_ + __ebs_, __extbuf_end);
- if (__end == __b)
- return traits_type::eof();
+ if (__end == __b) {
+ return __failed();
+ }
// No conversion needed: output characters directly to the file, done.
if (__r == codecvt_base::noconv) {
size_t __n = static_cast<size_t>(__p - __b);
- if (std::fwrite(__b, 1, __n, __file_) != __n)
- return traits_type::eof();
+ if (std::fwrite(__b, 1, __n, __file_) != __n) {
+ return __failed();
+ }
break;
// Conversion successful: output the converted characters to the file, done.
} else if (__r == codecvt_base::ok) {
size_t __n = static_cast<size_t>(__extbuf_end - __extbuf_);
- if (std::fwrite(__extbuf_, 1, __n, __file_) != __n)
- return traits_type::eof();
+ if (std::fwrite(__extbuf_, 1, __n, __file_) != __n) {
+ return __failed();
+ }
break;
// Conversion partially successful: output converted characters to the file and repeat with the
// remaining characters.
} else if (__r == codecvt_base::partial) {
size_t __n = static_cast<size_t>(__extbuf_end - __extbuf_);
- if (std::fwrite(__extbuf_, 1, __n, __file_) != __n)
- return traits_type::eof();
+ if (std::fwrite(__extbuf_, 1, __n, __file_) != __n) {
+ return __failed();
+ }
__b = const_cast<char_type*>(__end);
continue;
} else {
- return traits_type::eof();
+ return __failed();
}
} while (true);
}
diff --git a/libcxx/test/std/input.output/file.streams/fstreams/filebuf.virtuals/overflow.writefail.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/filebuf.virtuals/overflow.writefail.pass.cpp
new file mode 100644
index 0000000000000..27e06982d749b
--- /dev/null
+++ b/libcxx/test/std/input.output/file.streams/fstreams/filebuf.virtuals/overflow.writefail.pass.cpp
@@ -0,0 +1,72 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: no-filesystem
+
+// setrlimit(RLIMIT_FSIZE) seems to only work as intended on Apple platforms
+// REQUIRES: target={{.+}}-apple-{{.+}}
+
+// <fstream>
+
+// Make sure that we properly handle the case where we try to write content to a file
+// but we fail to do so because std::fwrite fails.
+
+#include <cassert>
+#include <csignal>
+#include <cstddef>
+#include <fstream>
+#include <string>
+
+#include "platform_support.h"
+#include "test_macros.h"
+
+#if __has_include(<sys/resource.h>)
+# include <sys/resource.h>
+void limit_file_size_to(std::size_t bytes) {
+ rlimit lim = {bytes, bytes};
+ assert(setrlimit(RLIMIT_FSIZE, &lim) == 0);
+
+ std::signal(SIGXFSZ, [](int) {}); // ignore SIGXFSZ to ensure std::fwrite fails
+}
+#else
+# error No known way to limit the amount of filesystem space available
+#endif
+
+template <class CharT>
+void test() {
+ std::string temp = get_temp_file_name();
+ std::basic_filebuf<CharT> fbuf;
+ assert(fbuf.open(temp, std::ios::out | std::ios::trunc));
+
+ std::size_t const limit = 100000;
+ limit_file_size_to(limit);
+
+ std::basic_string<CharT> large_block(limit / 10, CharT(42));
+
+ std::streamsize ret;
+ std::size_t bytes_written = 0;
+ while ((ret = fbuf.sputn(large_block.data(), large_block.size())) != 0) {
+ bytes_written += ret;
+
+ // In theory, it's possible for an implementation to allow writing arbitrarily more bytes than
+ // set by setrlimit, but in practice if we bust 100x our limit, something else is wrong with the
+ // test and we'd end up looping forever.
+ assert(bytes_written < 100 * limit);
+ }
+
+ fbuf.close();
+}
+
+int main(int, char**) {
+ test<char>();
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+ test<wchar_t>();
+#endif
+
+ return 0;
+}
|
@tru For context: we have originally requested a cherry-pick of this around the time it was merged to the main branch, but it looks like the bot wasn't triggered for some reason, and I don't see this commit in the release branch (see e.g. the log for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should backport #149390 as well.
Backport 6291b63
Requested by: @var-const