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

[David Holmes]

On 5/06/2019 6:42 pm, Thomas Stüfe wrote:
> On Wed, Jun 5, 2019 at 10:08 AM David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
> 
>     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>
>      > <mailto: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.
> 
> 
> Can I read this as a reluctantly assenting growl or a negative? :)

Reluctantly assenting :)

Thanks,
David

> 
> 
>      >
>      >     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.
> 
> 
> I will fix this and post an updated webrev.
> 
> ..Thomas
> 
> 
>     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
>      >      >
>      >
>