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

[Volodymyr Sapsai via cfe-commits]

On Nov 9, 2017, at 12:13, Reimar Döffinger <Reimar.Doeffinger at gmx.de> 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 <iostream>
> #include <sstream>
> #include <string>
> 
> 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 <cfe-commits at lists.llvm.org> 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 at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> 
> <0001-Ensure-std-istream-getline-always-0-terminates-strin.patch>