RFR (L): 8062370: Various minor code improvements
David Holmes
david.holmes at oracle.com
Fri Nov 7 07:13:10 UTC 2014
Hi Thomas,
On 6/11/2014 11:21 PM, Thomas Stüfe wrote:
> Hi David,
>
> Our intend was to always guarantee that the written string is zero
> terminated, and across platforms to always return the same value (-1) if
> truncation happened.
>
> The original jio_snprintf() did not have any value over plain snprintf()
> (apart from maybe solving the name problem with "_snprintf" on Windows).
> But having a dedicated wrapper function around snprintf() suggests some
> added value, and I always thought that value was supposed to be zero
> termination. If that is not intended, it would be better to use the
> plain snprintf instead, because at least C programmers then know what to
> expect.
>
> I also found very few cases where the return code of jio_snprintf() was
> actually checked and truncation handled correctly. Which would be
> difficult too, because the return code differed for truncation between
> windows and Posix.
>
> Bottomline, I think it would be better if jio_snprintf() were to always
> zero-terminate, guaranteed.
I don't disagree, but we needed to make sure that everyone was on the
same page before this change occurred. It is not unreasonable that
Windows developers writing windows code assumed windows semantics. As a
Reviewer I should have paid more attention to this aspect.
Also if we are going to implement this behaviour then it really needs to
be clearly documented - both in terms of null-termination and return
value - and commented in the code (anyone knowing *NIX or Windows
behaviour alone will be quite perplexed by it I think). Further,
returning -1 to indicate truncation while familiar to windows
programmers might be frustrating to others who would like to know what
size buffer was needed - lowest common denominator I know :(
And as you note most uses of this function have a "dont care" attitude
to truncation - which makes it hard to spot if there may be other
lurking truncation issues in the windows code.
Cheers,
David
> Kind Regards, Thomas
>
>
> On Thu, Nov 6, 2014 at 12:23 PM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> 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
> <mailto:david.holmes at oracle.com>]
> Sent: Donnerstag, 6. November 2014 11:30
> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
> <mailto: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
> <mailto:david.holmes at oracle.com>]
> Sent: Donnerstag, 6. November 2014 11:09
> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
> <mailto: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/
> <http://cr.openjdk.java.net/~goetz/webrevs/8062370/webrev.01/>
>
> Best regards,
> Goetz.
>
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.__com
> <mailto:david.holmes at oracle.com>]
> Sent: Mittwoch, 5. November 2014 02:49
> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
> <mailto: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/
> <http://cr.openjdk.java.net/~goetz/webrevs/8062370/webrev.00/>
> https://bugs.openjdk.java.net/__browse/JDK-8062370
> <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
>
>
>
>
>
More information about the hotspot-dev
mailing list