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

[David Holmes]

On 5/06/2019 5:43 pm, Thomas Stüfe wrote:
> Hi David,
> 
> 
> 
> On Wed, Jun 5, 2019 at 9:38 AM David Holmes <david.holmes at oracle.com 
> <mailto: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.

Hmmm okay.

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

I think code-checkers might still be more happy with the strncpy.

Thanks,
David

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