On Wed, Nov 15, 2017 at 11:35:56AM -0800, Volodymyr Sapsai wrote:
> On Nov 12, 2017, at 12:37, Reimar Döffinger <reimar.doeffin...@gmx.de> wrote:
> libc++ can be built with exceptions enabled or disabled (see
> LIBCXX_ENABLE_EXCEPTIONS
> <http://libcxx.llvm.org/docs/BuildingLibcxx.html#cmdoption-arg-libcxx-enable-exceptions>)
> and we need to support both configurations. The problem with your
> _LIBCPP_NO_EXCEPTIONS approach is that it won't work when exceptions are
> disabled. Various exception-related tests are usually guarded with
> TEST_HAS_NO_EXCEPTIONS macro defined in “test_macros.h”. You can check other
> tests for examples of TEST_HAS_NO_EXCEPTIONS usage, I had in mind something
> like
>
> #ifndef TEST_HAS_NO_EXCEPTIONS
> {
> testbuf<char> sb(" ");
> std::istream is(&sb);
> char s[5] = "test";
> is.exceptions(std::istream::eofbit | std::istream::badbit);
> try
> {
> is.getline(s, 5);
> assert(false);
That doesn't make sense to me, the whole point is to test the
code-path in getline that swallows the exception.
So if you meant for that assert to be there, you're
not trying to test what I am trying to test.
Note that the intent is not to ensure 0-termination
when getline actually exits via an exception, I doubt
that would be safe to do really (except maybe at the start
of the loop before the code that might trigger an exception,
but that would cost performance) and anyway code
handling exceptions can be expected to not make assumptions
that the result is in a "safe" state.
The problem is when the code just swallows the exception
(when exceptions are disabled, presumably for interoperability
with code compiled with exceptions enabled? There is no documentation
why getline catches exceptions when they are disabled),
then I think the only reasonable choice is to 0-terminate,
even if it is risky.
Looking at the other tests it seems that generally functionality
simply isn't tested at all when exceptions are disabled
(e.g. stoul.pass.cpp), which seems unfortunate.
That level of testing is easy to achieve here as well,
just leave out the test file I added.
If you want it though, I updated it to not fail (though
not actually test anything either) if exceptions are
disabled, and added a comment on what a huge hack it is
(as the code-path for disabled exceptions is tested if and
only if exceptions are actually enabled).
>From b241434a555dcde485f604b1393805f585d68d7a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Reimar=20D=C3=B6ffinger?= <reimar.doeffin...@gmx.de>
Date: Thu, 7 Sep 2017 08:42:10 +0200
Subject: [PATCH] Ensure std::istream::getline always 0-terminates string.
If the sentinel failed (e.g. due to having reached
EOF before) or an exception was caught it failed to
do that.
The C++14 standard says:
"In any case, if n is greater than zero, it then stores
a null character (using charT()) into the next
successive location of the array."
Other implementations like libstdc++ do 0-terminate and
not doing so may lead security issues in applications.
Note that means behaviour is inconsistent with the
std::getline version returning a std::string, which
does not modify the string in case of error, but
that is both less serious and matches behaviour
of e.g. libstdc++, so leave it as-is for now.
---
include/istream | 6 +-
.../getline_pointer_size.pass.cpp | 14 +++++
.../getline_pointer_size_chart.pass.cpp | 14 +++++
.../getline_pointer_size_exception.pass.cpp | 68 ++++++++++++++++++++++
4 files changed, 100 insertions(+), 2 deletions(-)
create mode 100644 test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_exception.pass.cpp
diff --git a/include/istream b/include/istream
index 0b8e05d95..5c73df38f 100644
--- a/include/istream
+++ b/include/istream
@@ -1069,16 +1069,18 @@ basic_istream<_CharT, _Traits>::getline(char_type* __s, streamsize __n, char_typ
this->rdbuf()->sbumpc();
++__gc_;
}
- if (__n > 0)
- *__s = char_type();
if (__gc_ == 0)
__err |= ios_base::failbit;
this->setstate(__err);
}
+ if (__n > 0)
+ *__s = char_type();
#ifndef _LIBCPP_NO_EXCEPTIONS
}
catch (...)
{
+ if (__n > 0)
+ *__s = char_type();
this->__set_badbit_and_consider_rethrow();
}
#endif // _LIBCPP_NO_EXCEPTIONS
diff --git a/test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size.pass.cpp b/test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size.pass.cpp
index 465824a65..d0da78b5c 100644
--- a/test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size.pass.cpp
+++ b/test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size.pass.cpp
@@ -59,6 +59,13 @@ int main()
assert(!is.fail());
assert(std::string(s) == " ");
assert(is.gcount() == 1);
+ is.getline(s, 5);
+ // check that even in error case the buffer
+ // is properly 0-terminated
+ assert( is.eof());
+ assert( is.fail());
+ assert(std::string(s) == "");
+ assert(is.gcount() == 0);
}
{
testbuf<wchar_t> sb(L" \n \n ");
@@ -79,5 +86,12 @@ int main()
assert(!is.fail());
assert(std::wstring(s) == L" ");
assert(is.gcount() == 1);
+ is.getline(s, 5);
+ // check that even in error case the buffer
+ // is properly 0-terminated
+ assert( is.eof());
+ assert( is.fail());
+ assert(std::wstring(s) == L"");
+ assert(is.gcount() == 0);
}
}
diff --git a/test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_chart.pass.cpp b/test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_chart.pass.cpp
index 736295996..084104110 100644
--- a/test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_chart.pass.cpp
+++ b/test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_chart.pass.cpp
@@ -59,6 +59,13 @@ int main()
assert(!is.fail());
assert(std::string(s) == " ");
assert(is.gcount() == 1);
+ is.getline(s, 5, '*');
+ // check that even in error case the buffer
+ // is properly 0-terminated
+ assert( is.eof());
+ assert( is.fail());
+ assert(std::string(s) == "");
+ assert(is.gcount() == 0);
}
{
testbuf<wchar_t> sb(L" * * ");
@@ -79,5 +86,12 @@ int main()
assert(!is.fail());
assert(std::wstring(s) == L" ");
assert(is.gcount() == 1);
+ is.getline(s, 5, L'*');
+ // check that even in error case the buffer
+ // is properly 0-terminated
+ assert( is.eof());
+ assert( is.fail());
+ assert(std::wstring(s) == L"");
+ assert(is.gcount() == 0);
}
}
diff --git a/test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_exception.pass.cpp b/test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_exception.pass.cpp
new file mode 100644
index 000000000..76a4b813d
--- /dev/null
+++ b/test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_exception.pass.cpp
@@ -0,0 +1,68 @@
+//===----------------------------------------------------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// <istream>
+
+// basic_istream<charT,traits>& getline(char_type* s, streamsize n);
+
+// This is rather stupid: to properly test the special code path
+// for disabled exceptions we actually need to trigger an exception,
+// and as a result can run the test only when exceptions are enabled.
+// I.e. the exceptions disabled code-path is tested if and only if exceptions
+// are actually enabled.
+// This means this test only provides code-coverage in a use-case
+// that does not match the intended environment. Still, seems better
+// than nothing?
+
+#define _LIBCPP_NO_EXCEPTIONS 1
+#include <stdexcept>
+#include <istream>
+#include <cassert>
+
+#include "test_macros.h"
+
+#ifdef TEST_HAS_NO_EXCEPTIONS
+int main()
+{}
+#else
+template <class CharT>
+struct throwing_testbuf
+ : public std::basic_streambuf<CharT>
+{
+ virtual typename std::basic_streambuf<CharT>::int_type underflow() { throw std::runtime_error(""); }
+};
+
+int main()
+{
+ {
+ throwing_testbuf<char> sb;
+ std::istream is(&sb);
+ char s[5];
+ is.getline(s, 5);
+ // check that even in error case the buffer
+ // is properly 0-terminated
+ assert(!is.eof());
+ assert( is.fail());
+ assert(std::string(s) == "");
+ assert(is.gcount() == 0);
+ }
+ {
+ throwing_testbuf<wchar_t> sb;
+ std::wistream is(&sb);
+ wchar_t s[5];
+ is.getline(s, 5);
+ // check that even in error case the buffer
+ // is properly 0-terminated
+ assert(!is.eof());
+ assert( is.fail());
+ assert(std::wstring(s) == L"");
+ assert(is.gcount() == 0);
+ }
+}
+#endif
--
2.15.0
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
