RFR(xs): 8225225: stringStream internal buffer should always be zero terminated

[Thomas Stüfe]

Hi David,



On Wed, Jun 5, 2019 at 9:38 AM David Holmes <david.holmes at oracle.com> wrote:

> Hi Thomas,
>
> On 4/06/2019 6:06 pm, Thomas Stüfe wrote:
> > Hi all,
> >
> > please review this small fix and the associated test:
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8225225
> > Webrev:
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8225225-stringstream-should-always-be-zero-terminated/webrev.00/webrev/
> >
> > This patch makes sure the internal buffer of stringStream is always zero
> > terminated.
>
> I don't see why a newly constructed empty stringstream needs to be
> zero-terminated. We ensure zero termination if we write, and we ensure
> we zero-terminate when we convert via as_string. If someone uses base()
> to directly access our internal buffer (why do we allow that??) then
> they need to be marginally careful when size() is 0.
>

This is a preparation patch for
https://bugs.openjdk.java.net/browse/JDK-8224212 . I posted a separate RFR
for that one.

Calling stringStream::base() is a valid way to access the assembled string
and often more efficient than as_string(). Especially in cases where the
stringStream has been used to assemble a larger report.

That is why I want to guarantee that ::base() returns always a zero
terminated string, regardless the stream state.


> But if you do make this change then surely here:
>
>   370 char* stringStream::as_string() const {
>   371   char* copy = NEW_RESOURCE_ARRAY(char, buffer_pos + 1);
>   372   strncpy(copy, buffer, buffer_pos);
>   373   copy[buffer_pos] = 0;  // terminating null
>
> we don't need to do this as we must have copied across a terminating
> NUL? (Actually doesn't that apply now, other than when size() == 0?)
>

You are right. Also, a simple strcpy would be sufficient since the
receiving buffer is large enough.

Cheers, Thomas


>
> Thanks,
> David
>
> > It also extends existing gtests to assert this.
> >
> > In addition, gtests are expanded to test fixed sized stringStream
> variants
> > too, and to test for buffer overflow for both bufferedStream and
> > stringStream (not strictly related to this fix but I was in the area
> so..).
> >
> > Thanks, Thomas
> >
>