RFR (L): 8062370: Various minor code improvements

Wed Nov 12 10:31:37 UTC 2014

Hi,

could you please review this little addition? (added comments for
jio_snprintf)

http://cr.openjdk.java.net/~simonis/webrevs/8062370/

Thanks!

Best Regards,

Thomas

On Tue, Nov 11, 2014 at 11:37 AM, Markus Grönlund <
markus.gronlund at oracle.com> wrote:

> Hi Goetz,
>
> Thanks for following up on this.
>
> I adjusted a few calls into jio_snprintf() that were particular for
> Windows to accommodate the updates.
>
> From the test results I have seen so far, it seems no other issues
> appeared which could be related to this.
>
> So I think the change should be left as is (no rollback), but maybe a
> comment could be added about the jio_snprintf() semantics (NULL termination
> on overflows, expected 'count' etc).
>
> Thanks
> Markus
>
>
> -----Original Message-----
> From: Lindenmaier, Goetz [mailto:goetz.lindenmaier at sap.com]
> Sent: den 11 november 2014 11:14
> To: Markus Grönlund
> Cc: David Holmes; hotspot-dev at openjdk.java.net
> Subject: RE: RFR (L): 8062370: Various minor code improvements
>
> Hi Markus,
>
> Could you fix your issue and did the other tests pass?
>
> Are there any follow-up actions I should take?
>
> Best regards,
>   Goetz.
>
> -----Original Message-----
> From: Markus Grönlund [mailto:markus.gronlund at oracle.com]
> Sent: Donnerstag, 6. November 2014 14:50
> To: Lindenmaier, Goetz
> Cc: David Holmes; hotspot-dev at openjdk.java.net
> Subject: RE: RFR (L): 8062370: Various minor code improvements
>
> Hi Goetz,
>
> Thanks for looking into this.
>
> I think I will be able to update the internal code I am working on to
> accommodate your updates.
>
> I don't know if any other code will see potential issues - only testing
> will tell.
>
> So I would await the rollback and I will putback my updated code - let's
> see if other issues appear after this - we should know after this nights
> nightly testing (then we can re-evaluate the rollback).
>
> Thanks
> Markus
>
>
> -----Original Message-----
> From: Lindenmaier, Goetz [mailto:goetz.lindenmaier at sap.com]
> Sent: den 6 november 2014 13:49
> To: David Holmes; hotspot-dev at openjdk.java.net
> Cc: Markus Grönlund
> Subject: RE: RFR (L): 8062370: Various minor code improvements
>
> Hi David,
>
> Well, yes, that's right.  But then you can simply pass in count+1.
> It works also if the caller knows he will only use 'count' bytes of the
> string.  In this case +1 must be allocated.
> But that both is quite special.
>
> Currently, if the string is truncated, there is no null byte on windows.
> And there are a lot of uses of this method in the VM (via jio_snprintf).
>
> Should I use the internal bug number for the rollback-fix?
>
> How should we proceed, as I can't fix you internal code?
>
> Best regards,
>   Goetz.
>
>
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Donnerstag, 6. November 2014 12:23
> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
> Cc: Markus Grönlund
> Subject: Re: RFR (L): 8062370: Various minor code improvements
>
> On 6/11/2014 8:43 PM, Lindenmaier, Goetz wrote:
> > Hi David,
> >
> > yes, windows does not null terminate if there is an overflow.
> > Obviously there are overflows, and they now see one less character.  I
> > think this should be fixed where jio_vsnprintf is called.  Having
> > non-null terminated strings isn't nice.
>
> I think it depends on what you consider an overflow. If the buffer is
> already null terminated and you pass in a count that covers up to the
> location before the null then there is no problem - except now the logic
> will introduce a second null in place of the last character.
>
> > But for now I will roll back this single change.  I'll send a RFR soon.
> >
> > Where did you see the problem?
>
> It was in our closed code so I can't go into details. We have a non-public
> bug number: 8063089
>
> Thanks,
> David
>
> >
> > Best regards,
> >    Goetz.
> >
> >
> >
> >
> > -----Original Message-----
> > From: David Holmes [mailto:david.holmes at oracle.com]
> > Sent: Donnerstag, 6. November 2014 11:30
> > To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
> > Cc: Markus Grönlund
> > Subject: Re: RFR (L): 8062370: Various minor code improvements
> >
> > On 6/11/2014 8:17 PM, Lindenmaier, Goetz wrote:
> >> Thanks David, I'll have a look.
> >
> > It seems that windows vsnprintf may not null-terminate the string -
> > which I think is what your patch was trying to address. But if we have
> > existing code that works with that then the fix is now overwriting the
> > last character. I can't quite see how to handle this in a cross
> > platform manner, but in the immediate term we should probably revert
> > that part of the changeset.
> >
> > David
> >
> >> Best regards,
> >>     Goetz.
> >>
> >> -----Original Message-----
> >> From: David Holmes [mailto:david.holmes at oracle.com]
> >> Sent: Donnerstag, 6. November 2014 11:09
> >> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
> >> Cc: Markus Grönlund
> >> Subject: Re: RFR (L): 8062370: Various minor code improvements
> >>
> >> Hi Goetz,
> >>
> >> This change has introduced a bug:
> >>
> >> -  return vsnprintf(str, count, fmt, args);
> >> +
> >> +  int result = vsnprintf(str, count, fmt, args);  if ((result > 0 &&
> >> + (size_t)result >= count) || result == -1) {
> >> +    str[count - 1] = '\0';
> >> +    result = -1;
> >> +  }
> >> +
> >> +  return result;
> >>
> >> some strings are getting their last character truncated on Windows.
> >>
> >> David
> >>
> >> On 5/11/2014 6:16 PM, Lindenmaier, Goetz wrote:
> >>> Hi David,
> >>>
> >>> thanks for looking at the change!  I fixed the issue in a new
> >>> webrev:
> >>> http://cr.openjdk.java.net/~goetz/webrevs/8062370/webrev.01/
> >>>
> >>> Best regards,
> >>>      Goetz.
> >>>
> >>> -----Original Message-----
> >>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>> Sent: Mittwoch, 5. November 2014 02:49
> >>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
> >>> Subject: Re: RFR (L): 8062370: Various minor code improvements
> >>>
> >>> Hi Goetz,
> >>>
> >>> The only issue I see is in:
> >>>
> >>> src/share/vm/runtime/globals.cpp
> >>>
> >>> where you replaced NEW_C_HEAP_ARRAY with os::strdup. To keep the
> >>> "abort on OOM" semantics of NEW_C_HEAP_ARRAY you need to use
> os::strdup_check_oom.
> >>>
> >>> Thanks,
> >>> David
> >>>
> >>> On 30/10/2014 6:28 PM, Lindenmaier, Goetz wrote:
> >>>> Hi,
> >>>>
> >>>> this change contains a row of minor code improvements we did to
> >>>> fulfil our internal quality requirements.  We would like to share
> >>>> these with openJDK.
> >>>>
> >>>> Please review and test this change.  I please need a sponsor.
> >>>> http://cr.openjdk.java.net/~goetz/webrevs/8062370/webrev.00/
> >>>> https://bugs.openjdk.java.net/browse/JDK-8062370
> >>>>
> >>>> We tested this on windows 64, linux x86_64, mac, solaris sparc
> >>>> 32+64 bit and, of course, the ppc platforms.
> >>>>
> >>>>
> >>>> Some details:
> >>>>
> >>>> CONST64(0x8000000000000000) is wrong, as 0x8... is positive, and thus
> not representable as i64 what is used in the CONST64 macro. This change
> adapts UCONST64 to use ui64, and the usages of these macros where necessary.
> >>>>
> >>>> We add some more strncpy uses.  Also, we fix strncpy on windows.
> There, strncpy does not write a \0 into the last byte if the copied string
> is too long.
> >>>>
> >>>> We add some missing memory frees and some closing of files.
> >>>>
> >>>> jio_vsnprintf() works differently on windows and linux. This change
> adapts this to show the same behaviour on all platforms. See java.cpp.
> >>>>
> >>>> Best regards,
> >>>>
> >>>>       Goetz
> >>>>
> >>>>
> >>>>
> >>>>
>