Re: [PATCH] Ensure std::getline always 0-terminates string.

[Volodymyr Sapsai via cfe-commits]

On Nov 20, 2017, at 23:59, Reimar Döffinger  wrote:
> 
> On 21.11.2017, at 03:54, Volodymyr Sapsai  > wrote:
>> 
>>> If (exceptions()) != 0 then the exception is rethrown.
>> And looks like libstdc++ rethrows exception even if badbit is not set.
> 
> Thanks for looking all that up, I was in a hurry.
> 
>> If you feel comfortable, I can finish exception tests myself and commit the 
>> patch. How does it sound?
> 
> That would be very welcome if you don't mind.
> I don't have much time over to spend on this most days.
> 
> Thanks,
> Reimar Döffinger

Committed in r318862 and r318863 (the latter one is my mistake). Thanks for 
contributing the fix.

Regards,
Volodymyr___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] Ensure std::getline always 0-terminates string.

[Reimar Döffinger via cfe-commits]

On 21.11.2017, at 03:54, Volodymyr Sapsai  wrote:
> 
>> If (exceptions()) != 0 then the exception is rethrown.
> And looks like libstdc++ rethrows exception even if badbit is not set.

Thanks for looking all that up, I was in a hurry.

> If you feel comfortable, I can finish exception tests myself and commit the 
> patch. How does it sound?

That would be very welcome if you don't mind.
I don't have much time over to spend on this most days.

Thanks,
Reimar Döffinger___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] Ensure std::getline always 0-terminates string.

[Volodymyr Sapsai via cfe-commits]

On Nov 20, 2017, at 16:33, Reimar Döffinger  wrote:
> 
> On 20 November 2017 22:19:04 CET, Volodymyr Sapsai  > wrote:
>> On Nov 20, 2017, at 11:32, Reimar Döffinger 
>> wrote:
>>> 
>>> On Mon, Nov 20, 2017 at 11:02:13AM -0800, Volodymyr Sapsai wrote:
>catch (...)
>{
> +if (__n > 0)
> +*__s = char_type();
>this->__set_badbit_and_consider_rethrow();
>}
 
 or maybe something else?
>>> 
>>> That one (note that the __set_badbit_and_consider_rethrow
>>> will never re-throw in this case).
>> 
>> But by #define _LIBCPP_NO_EXCEPTIONS 1 you exclude this block at
>> preprocessing step
>> 
>>> #ifndef _LIBCPP_NO_EXCEPTIONS
>>> }
>>> catch (...)
>>> {
>>> +if (__n > 0)
>>> +*__s = char_type();
>>> this->__set_badbit_and_consider_rethrow();
>>> }
>>> #endif  // _LIBCPP_NO_EXCEPTIONS
>> 
>> And looks like getline_pointer_size_exception.pass.cpp doesn’t execute
>> this code.
> 
> Yes, that was complete nonsense, I somehow always read #ifdef where there was 
> an #ifndef...
> Not sure then if that 0 termination should be there (and be tested) or better 
> to not have it in the exception case at all (I haven't checked if the 
> exception is always re-thrown or not, which might be relevant).
> I am a bit unclear about that whole code there that catches exceptions and 
> sets the bad bit and how it maps to the specification.

Standard mentions

> Otherwise, if the sentry constructor exits by throwing an exception or if the 
> sentry object returns false, when converted to a value of type bool, the 
> function returns without attempting to obtain any input. In either case the 
> number of extracted characters is set to 0; unformatted input functions 
> taking a character array of non-zero size as an argument shall also store a 
> null character (using charT()) in the first location of the array.

My understanding is that if sentry constructor throws an exception we need to 
null-terminate a character array. Null-termination isn’t specified explicitly 
for exceptions thrown in other cases but I think it would be more cumbersome to 
perform null-termination only for sentry exceptions and not for others. 
Regarding the safety, we control __s, don’t pass it anywhere, it never points 
beyond __n - I think null-termination should be safe. Also libstdc++ does 
null-termination when exceptions are thrown.

As for rethrowing exception, looks like exceptions flag std::istream::eofbit 
causes exception to be thrown and std::istream::badbit to be rethrown according 
to
> If (exceptions()) != 0 then the exception is rethrown.
And looks like libstdc++ rethrows exception even if badbit is not set.

If you feel comfortable, I can finish exception tests myself and commit the 
patch. How does it sound?___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] Ensure std::getline always 0-terminates string.

[Reimar Döffinger via cfe-commits]

On 20 November 2017 22:19:04 CET, Volodymyr Sapsai  wrote:
>On Nov 20, 2017, at 11:32, Reimar Döffinger 
>wrote:
>> 
>> On Mon, Nov 20, 2017 at 11:02:13AM -0800, Volodymyr Sapsai wrote:
 catch (...)
 {
 +if (__n > 0)
 +*__s = char_type();
 this->__set_badbit_and_consider_rethrow();
 }
>>> 
>>> or maybe something else?
>> 
>> That one (note that the __set_badbit_and_consider_rethrow
>> will never re-throw in this case).
>
>But by #define _LIBCPP_NO_EXCEPTIONS 1 you exclude this block at
>preprocessing step
>
>>  #ifndef _LIBCPP_NO_EXCEPTIONS
>>  }
>>  catch (...)
>>  {
>> +if (__n > 0)
>> +*__s = char_type();
>>  this->__set_badbit_and_consider_rethrow();
>>  }
>>  #endif  // _LIBCPP_NO_EXCEPTIONS
>
>And looks like getline_pointer_size_exception.pass.cpp doesn’t execute
>this code.

Yes, that was complete nonsense, I somehow always read #ifdef where there was 
an #ifndef...
Not sure then if that 0 termination should be there (and be tested) or better 
to not have it in the exception case at all (I haven't checked if the exception 
is always re-thrown or not, which might be relevant).
I am a bit unclear about that whole code there that catches exceptions and sets 
the bad bit and how it maps to the specification.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] Ensure std::getline always 0-terminates string.

[Volodymyr Sapsai via cfe-commits]

On Nov 20, 2017, at 11:32, Reimar Döffinger  wrote:
> 
> On Mon, Nov 20, 2017 at 11:02:13AM -0800, Volodymyr Sapsai wrote:
>>> catch (...)
>>> {
>>> +if (__n > 0)
>>> +*__s = char_type();
>>> this->__set_badbit_and_consider_rethrow();
>>> }
>> 
>> or maybe something else?
> 
> That one (note that the __set_badbit_and_consider_rethrow
> will never re-throw in this case).

But by #define _LIBCPP_NO_EXCEPTIONS 1 you exclude this block at preprocessing 
step

>  #ifndef _LIBCPP_NO_EXCEPTIONS
>  }
>  catch (...)
>  {
> +if (__n > 0)
> +*__s = char_type();
>  this->__set_badbit_and_consider_rethrow();
>  }
>  #endif  // _LIBCPP_NO_EXCEPTIONS

And looks like getline_pointer_size_exception.pass.cpp doesn’t execute this 
code.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] Ensure std::getline always 0-terminates string.

[Reimar Döffinger via cfe-commits]

On Mon, Nov 20, 2017 at 11:02:13AM -0800, Volodymyr Sapsai wrote:
> >  catch (...)
> >  {
> > +if (__n > 0)
> > +*__s = char_type();
> >  this->__set_badbit_and_consider_rethrow();
> >  }
> 
> or maybe something else?

That one (note that the __set_badbit_and_consider_rethrow
will never re-throw in this case).
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] Ensure std::getline always 0-terminates string.

[Volodymyr Sapsai via cfe-commits]

On Nov 19, 2017, at 08:07, Reimar Döffinger  wrote:
> 
> On Wed, Nov 15, 2017 at 11:35:56AM -0800, Volodymyr Sapsai wrote:
>> On Nov 12, 2017, at 12:37, Reimar Döffinger  wrote:
>> libc++ can be built with exceptions enabled or disabled (see 
>> 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 sb(" ");
>>std::istream is();
>>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).
> <0001-Ensure-std-istream-getline-always-0-terminates-strin.patch>

I got confused here. When you mention "code path for disabled exceptions”, what 
exactly code do you have in mind? Is that

> -if (__n > 0)
> -*__s = char_type();
>  if (__gc_ == 0)
> __err |= ios_base::failbit;
>  this->setstate(__err);
>  }
> +if (__n > 0)
> +*__s = char_type();

or

>  catch (...)
>  {
> +if (__n > 0)
> +*__s = char_type();
>  this->__set_badbit_and_consider_rethrow();
>  }

or maybe something else?
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] Ensure std::getline always 0-terminates string.

[Reimar Döffinger via cfe-commits]

On Wed, Nov 15, 2017 at 11:35:56AM -0800, Volodymyr Sapsai wrote:
> On Nov 12, 2017, at 12:37, Reimar Döffinger  wrote:
> libc++ can be built with exceptions enabled or disabled (see 
> 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 sb(" ");
> std::istream is();
> 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?= 
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
+++ 

Re: [PATCH] Ensure std::getline always 0-terminates string.

[Volodymyr Sapsai via cfe-commits]

On Nov 12, 2017, at 12:37, Reimar Döffinger  wrote:
> 
> On Thu, Nov 09, 2017 at 05:37:32PM -0800, Volodymyr Sapsai wrote:
>> On Nov 9, 2017, at 12:13, Reimar Döffinger  wrote:
>>> 
>>> Hello!
>>> 
>>> On Wed, Nov 08, 2017 at 12:36:00PM -0800, Volodymyr Sapsai wrote:
 Thanks for the patch, Reimar. Can you please add tests to ensure this 
 functionality doesn’t regress? As null character is required by the 
 standard (27.7.2.3p21), a good starting point seems to be
 test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size.pass.cpp
 test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_chart.pass.cpp
>>> 
>>> New patch attached, though I went the lazy way of just adding one case.
>>> Not sure that's "good enough" - in principle I think it should be.
>> I think it makes sense to cover wchar_t as well. And I think it would be 
>> useful to test the case with specified delimiter too. Most likely 
>> implementation should be reused for the case with explicit delimiter and 
>> default new line delimiter. But I prefer not to rely on this assumption and 
>> test explicitly.
> 
> Well, it was rather trivial to do anyway, so done.
Thanks for adding more tests.

>>> More tricky would be to add a test for the _LIBCPP_NO_EXCEPTIONS case,
>>> is there any code testing that at all?
>> According to tests, exceptions are tested in various ways. Let me find how 
>> to configure the build to run tests in these modes.
> 
> There aren't many references to that mode, and I am not sure
> running in a separate mode would make for a good test anyway.
> Maybe not how it should done, but for now I added a separate file
> that just defines _LIBCPP_NO_EXCEPTIONS before the includes.
> <0001-Ensure-std-istream-getline-always-0-terminates-strin.patch>

libc++ can be built with exceptions enabled or disabled (see 
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 sb(" ");
std::istream is();
char s[5] = "test";
is.exceptions(std::istream::eofbit | std::istream::badbit);
try
{
is.getline(s, 5);
assert(false);
}
catch (std::ios_base::failure&)
{
}
assert( is.eof());
assert( is.fail());
assert(std::string(s) == " ");
assert(is.gcount() == 1);
}
#endif

Non-empty `sb` allows to test null-termination at correct place in array; 
initializing `s` with “test” avoids spurious correct values in memory; 
try-catch block and badbit are to make it more obvious we are testing code path 
throwing an exception.___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] Ensure std::getline always 0-terminates string.

[Volodymyr Sapsai via cfe-commits]

On Nov 9, 2017, at 12:13, Reimar Döffinger  wrote:
> 
> Hello!
> 
> On Wed, Nov 08, 2017 at 12:36:00PM -0800, Volodymyr Sapsai wrote:
>> Thanks for the patch, Reimar. Can you please add tests to ensure this 
>> functionality doesn’t regress? As null character is required by the standard 
>> (27.7.2.3p21), a good starting point seems to be
>> test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size.pass.cpp
>> test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_chart.pass.cpp
> 
> New patch attached, though I went the lazy way of just adding one case.
> Not sure that's "good enough" - in principle I think it should be.
I think it makes sense to cover wchar_t as well. And I think it would be useful 
to test the case with specified delimiter too. Most likely implementation 
should be reused for the case with explicit delimiter and default new line 
delimiter. But I prefer not to rely on this assumption and test explicitly.

> More tricky would be to add a test for the _LIBCPP_NO_EXCEPTIONS case,
> is there any code testing that at all?
According to tests, exceptions are tested in various ways. Let me find how to 
configure the build to run tests in these modes.

>> And what about free function std::getline that takes a stream and a string? 
>> The standard (21.4.8.9p7) specifies 
> [...]
>> 
>> Technically, string is not a character array of non-zero size. But according 
>> to the spirit of the standard, I would expect string to be empty after 
>> reading into it from a stream that reached EOF. What do you think?
> 
> Starting with the useful answer instead of what I think:
> 
> I'd try what other implementations do and align with that.
> At least libstdc++ does not clear the string, so tentatively
> I'd suggest not changing behaviour, for simple interoperability.
> Testing at least Microsoft C++ in addition might be a good idea
> though…
OK, let’s drop this case.

> Test code:
> 
> #include 
> #include 
> #include 
> 
> int main()
> {
>std::istringstream in;
>std::string dummy;
>in >> dummy;
>char test[20] = "oldstring";
>in.getline(test, sizeof(test));
>std::cout << "std::istream::getline result: " << test << std::endl;
>dummy = "oldstring";
>std::getline(in, dummy);
>std::cout << "std::getline result: " << dummy << std::endl;
>return test[0];
> }
> 
> 
> As to what I really think, for anyone who isn't tired of unfair rants
> (anyone else please just skip):
> 
> I don't really care much about that case, because at least unlike
> not 0-terminating a char buffer it is not in the "things you never EVER
> do" list.
> But this rather needs to be put to the standard authors and it needs to be
> clarified.
> Along with the suggestion to not only spend time on new features but
> also on improving the spec quality.
> Large parts of the spec read like when a second-semester maths student
> tries to write a proof: the language lacks the precision to actually
> write a solid proof, and excessive verbosity is used to try to make
> up for it (to no effect besides annoying the reader).
> The lack of specified pre- and post-conditions (specified formally
> or in natural language) for at least parts easy to specify in such a way
> to allow at least partial formal verification or proof of correctness
> isn't exactly state-of-the-art either.
> 
> Kind regards,
> Reimar Döffinger
> 
>>> On Oct 4, 2017, at 12:07, Reimar Döffinger via cfe-commits 
>>>  wrote:
>>> 
>>> 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.
>>> ---
>>> include/istream | 6 --
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>> 
>>> 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
>>> -- 
>>> 2.14.2
>>> 
>>> 

Re: [PATCH] Ensure std::getline always 0-terminates string.

[Reimar Döffinger via cfe-commits]

Hello!

On Wed, Nov 08, 2017 at 12:36:00PM -0800, Volodymyr Sapsai wrote:
> Thanks for the patch, Reimar. Can you please add tests to ensure this 
> functionality doesn’t regress? As null character is required by the standard 
> (27.7.2.3p21), a good starting point seems to be
> test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size.pass.cpp
> test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_chart.pass.cpp

New patch attached, though I went the lazy way of just adding one case.
Not sure that's "good enough" - in principle I think it should be.
More tricky would be to add a test for the _LIBCPP_NO_EXCEPTIONS case,
is there any code testing that at all?

> And what about free function std::getline that takes a stream and a string? 
> The standard (21.4.8.9p7) specifies 
[...]
> 
> Technically, string is not a character array of non-zero size. But according 
> to the spirit of the standard, I would expect string to be empty after 
> reading into it from a stream that reached EOF. What do you think?

Starting with the useful answer instead of what I think:

I'd try what other implementations do and align with that.
At least libstdc++ does not clear the string, so tentatively
I'd suggest not changing behaviour, for simple interoperability.
Testing at least Microsoft C++ in addition might be a good idea
though...
Test code:

#include 
#include 
#include 

int main()
{
std::istringstream in;
std::string dummy;
in >> dummy;
char test[20] = "oldstring";
in.getline(test, sizeof(test));
std::cout << "std::istream::getline result: " << test << std::endl;
dummy = "oldstring";
std::getline(in, dummy);
std::cout << "std::getline result: " << dummy << std::endl;
return test[0];
}


As to what I really think, for anyone who isn't tired of unfair rants
(anyone else please just skip):

I don't really care much about that case, because at least unlike
not 0-terminating a char buffer it is not in the "things you never EVER
do" list.
But this rather needs to be put to the standard authors and it needs to be
clarified.
Along with the suggestion to not only spend time on new features but
also on improving the spec quality.
Large parts of the spec read like when a second-semester maths student
tries to write a proof: the language lacks the precision to actually
write a solid proof, and excessive verbosity is used to try to make
up for it (to no effect besides annoying the reader).
The lack of specified pre- and post-conditions (specified formally
or in natural language) for at least parts easy to specify in such a way
to allow at least partial formal verification or proof of correctness
isn't exactly state-of-the-art either.

Kind regards,
Reimar Döffinger

> > On Oct 4, 2017, at 12:07, Reimar Döffinger via cfe-commits 
> >  wrote:
> > 
> > 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.
> > ---
> > include/istream | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > 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
> > -- 
> > 2.14.2
> > 
> > ___
> > cfe-commits mailing list
> > cfe-commits@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> 
>From 67ecbad84c70a611cb933b90bb10a10e5f32d4a4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Reimar=20D=C3=B6ffinger?= 
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 

Re: [PATCH] Ensure std::getline always 0-terminates string.

[Volodymyr Sapsai via cfe-commits]

Thanks for the patch, Reimar. Can you please add tests to ensure this 
functionality doesn’t regress? As null character is required by the standard 
(27.7.2.3p21), a good starting point seems to be
test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size.pass.cpp
test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_chart.pass.cpp

And what about free function std::getline that takes a stream and a string? The 
standard (21.4.8.9p7) specifies 
> […]After constructing a sentry object, if the sentry converts to true, calls 
> str.erase()[…]
It doesn’t mention explicitly effects to str if the sentry converts to false 
but it mentions that std::getline behaves as an unformatted input function 
(27.7.2.3). And 27.7.2.3p1 mentions
> […]If the sentry object returns true, when converted to a value of type bool, 
> the function endeavors to obtain the requested input. Otherwise, if the 
> sentry constructor exits by throwing an exception or if the sentry object 
> returns false, when converted to a value of type bool, the function returns 
> without attempting to obtain any input. In either case the number of 
> extracted characters is set to 0; unformatted input functions taking a 
> character array of non-zero size as an argument shall also store a null 
> character (using charT()) in the first location of the array.[…]


Technically, string is not a character array of non-zero size. But according to 
the spirit of the standard, I would expect string to be empty after reading 
into it from a stream that reached EOF. What do you think?

Regards,
Volodymyr

> On Oct 4, 2017, at 12:07, Reimar Döffinger via cfe-commits 
>  wrote:
> 
> 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.
> ---
> include/istream | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> 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
> -- 
> 2.14.2
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] Ensure std::getline always 0-terminates string.

[Reimar Döffinger via cfe-commits]

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.
---
 include/istream | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

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
-- 
2.14.2

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits