Re: 8215976: OpenJDK fails to build with GCC when the #include inside zip.cpp comes from a non-sysroot path

[David Holmes]

Mach 5 tier 1 - 3 tests passed on Linux/OS X/Windows x64 and Solaris 
sparcv9.


David

On 22/01/2019 2:01 pm, David Holmes wrote:

Hi Patrick,

I'm putting this through our test system (again) and will report back 
when its complete.


I also updated the bug report with the patch and assigned to Roger as 
the sponsor.


Cheers,
David

On 22/01/2019 12:21 pm, Patrick Zhang wrote:

Thanks Roger.

I did a try to push the patch within a branch to jdk/submit, and saw 
“Permission denied (publickey)”. I sent my SSH pub key to 
keys(at)openjdk.java.net but got no response. I heard only committers 
can have the access right, could you please help?


Regards

Patrick

*From:* Roger Riggs 
*Sent:* Wednesday, January 16, 2019 11:07 PM
*To:* Alan Bateman ; Patrick Zhang 
; David Holmes 

*Cc:* core-libs-dev 
*Subject:* Re: 8215976: OpenJDK fails to build with GCC when the 
#include inside zip.cpp comes from a non-sysroot path


Hi,

I can sponsor this. Let me know when the jdk-submit is complete.

Thanks, Roger

On 01/16/2019 10:03 AM, Alan Bateman wrote:

    On 16/01/2019 02:30, Patrick Zhang wrote:

    Looks .patch didn't work as an attachment, retry with .txt and
    paste the text below as well. Sorry for one more message here.

    I think this patch looks okay and I assume you'll test it in the
    jdk-submit repo to make sure it builds on all platforms.

    -Alan

Re: 8215976: OpenJDK fails to build with GCC when the #include inside zip.cpp comes from a non-sysroot path

[David Holmes]

Hi Patrick,

I'm putting this through our test system (again) and will report back 
when its complete.


I also updated the bug report with the patch and assigned to Roger as 
the sponsor.


Cheers,
David

On 22/01/2019 12:21 pm, Patrick Zhang wrote:

Thanks Roger.

I did a try to push the patch within a branch to jdk/submit, and saw 
“Permission denied (publickey)”. I sent my SSH pub key to 
keys(at)openjdk.java.net but got no response. I heard only committers 
can have the access right, could you please help?


Regards

Patrick

*From:* Roger Riggs 
*Sent:* Wednesday, January 16, 2019 11:07 PM
*To:* Alan Bateman ; Patrick Zhang 
; David Holmes 

*Cc:* core-libs-dev 
*Subject:* Re: 8215976: OpenJDK fails to build with GCC when the 
#include inside zip.cpp comes from a non-sysroot path


Hi,

I can sponsor this. Let me know when the jdk-submit is complete.

Thanks, Roger

On 01/16/2019 10:03 AM, Alan Bateman wrote:

On 16/01/2019 02:30, Patrick Zhang wrote:

Looks .patch didn't work as an attachment, retry with .txt and
paste the text below as well. Sorry for one more message here.

I think this patch looks okay and I assume you'll test it in the
jdk-submit repo to make sure it builds on all platforms.

-Alan

RE: 8215976: OpenJDK fails to build with GCC when the #include inside zip.cpp comes from a non-sysroot path

[Patrick Zhang]

Thanks Roger.

I did a try to push the patch within a branch to jdk/submit, and saw 
“Permission denied (publickey)”. I sent my SSH pub key to 
keys(at)openjdk.java.net but got no response. I heard only committers can have 
the access right, could you please help?

Regards
Patrick

From: Roger Riggs 
Sent: Wednesday, January 16, 2019 11:07 PM
To: Alan Bateman ; Patrick Zhang 
; David Holmes 
Cc: core-libs-dev 
Subject: Re: 8215976: OpenJDK fails to build with GCC when the #include 
inside zip.cpp comes from a non-sysroot path

Hi,

I can sponsor this. Let me know when the jdk-submit is complete.

Thanks, Roger

On 01/16/2019 10:03 AM, Alan Bateman wrote:
On 16/01/2019 02:30, Patrick Zhang wrote:

Looks .patch didn't work as an attachment, retry with .txt and paste the text 
below as well. Sorry for one more message here.
I think this patch looks okay and I assume you'll test it in the jdk-submit 
repo to make sure it builds on all platforms.

-Alan

Re: Manifest copy constructor does not deeply copy individual section Attributes

[Lance Andersen]

Hi Philipp,

This is going to take some further discussion/input as this has been documented 
for sometime as a shallow copy.

If there is a consensus  to move this forward, it will also require a CSR.   I 
can help with that and sponsoring the updates,  if the decision is to go ahead 
with the proposed change

Best
Lance
> On Jan 19, 2019, at 2:34 PM, Philipp Kunz  wrote:
> 
> Hi again,
> Just after having sent the previous mail I found Manifest::clone
> explicitly says it creates a shallow copy which is true only to a
> certain extent. It deeply copies main attributes as well as individual
> sections map already now and only shallowly copies individual sections
> attributes maps.
> I don't know about the background of it or why clone should only do a
> shallow copy but if clone should really create a shallow copy, it
> should probably create an even more shallow copy. In any case I figure
> some potential for clarification at least.
> Probably mostly because I already began a patch in the previous
> message, I continued and attached another patch for a deep copy
> approach. There might still be some reason not to deeply copy manifests
> which I don't intend to forestall.
> Philipp
> 
> On Sat, 2019-01-19 at 19:32 +0100, Philipp Kunz wrote:
>> Hi,
>> 
>> Creating a new manifest as a copy from an existing one using the copy
>> constructor assigns the new manifest individual sections entries map
>> the same values which are references to attributes as the original
>> rather than copying them as well deeply resulting in two manifests
>> pointing to the same attributes instances modifications to which
>> always affect both manifests. See test and proposed fix in the
>> attached patch.
>> 
>> Regards,
>> Philipp
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: High memory usage / leaks was: Best mailing list for JVM embedding

[David Holmes]

Hi Robert,

I've cc'd core-libs-dev as this is now about signed-jars and the launcher.

David

On 22/01/2019 4:48 am, Robert Marcano wrote:

On 1/21/19 8:25 AM, Robert Marcano wrote:

On 1/21/19 5:19 AM, Volker Simonis wrote:

-- Moved to hotspot-dev --

Hi Robert,

You can use "-XX:+PrintFlagsFinal" and compare the output for the two
variants to see if for some reason there are differing option
settings.


Thanks, compared the output of that on a java launcher call and my 
launcher and get the same flagsa values, so it doesn't look like 
different defaults isn't the problem


When testing this, trying to discard some weird Rust / OpenJDK 11 
interactions, I wrote a simpler test case using the JVM invocation API 
from plain C. Noticed the same pattern of high memory usage, but it 
allowed me to detect there was a difference when using the provided java 
launcher and our custom launcher.


Every VM option was the same (as strings), including the classpath. Both 
have something like -Djava.class.path=../lib/a.jar:../lib/b.jar


But for some error in the configuration of our test environment, ../lib 
pointed to different directories for both launchers. Different ../lib 
directories with the same JARs, the difference between them is that the 
use for the java launcher are unsigned and ../lib for the custom 
launcher are signed. These jars are signed because they come from a JNLP 
application, the new launcher is part of our migration out of JNLP.


So I managed to replicate the high memory usage using the standard java 
launcher.


So the question now is, why signed jars could affect the memory usage of 
an application (we aren't doing JAR verification on our custom launcher, 
yet), just by being on the java.class.path? IIRC the initial application 
classpath JARs were never verified previously (by the java launcher 
alone, without JNLP around).


Note: Tested with JARs signed with a self signed certificate and with 
one signed with a private CA. At most, signing the JARs could slow down 
the start up if it is now expected to these being verified by the java 
launcher (is it true?) but not higher memory usage and no reductions 
after a GC cycle but constants heap size increases.








Regards,
Volker

On Sat, Jan 19, 2019 at 6:23 PM Robert Marcano 
 wrote:


Greetings, which is the best mailing list for discussions about
embedding the JVM (via JNI_CreateJavaVM)?

The JVM is being embedded for desktop integration issues, for 
example to

show the appropriate application name on the process list instead of
java/java.exe, among many other things.

I am experiencing what looks like higher memory usage and/or failure to
garbage collect correctly when OpenJDK 11 is the embedded JVM. Starting
a test application using the java launcher, I get a GC log like this:


[0.007s][info][gc] Using G1
[0.389s][info][gc] GC(0) Pause Young (Normal) (G1 Evacuation Pause) 
10M->8M(124M) 8.661ms
[0.705s][info][gc] GC(1) Pause Young (Normal) (G1 Evacuation Pause) 
13M->10M(124M) 6.148ms

Jan 19, 2019 1:04:26 PM test.Test init
FINE: Starting application
[1.376s][info][gc] GC(3) Pause Young (Normal) (G1 Evacuation Pause) 
18M->10M(40M) 4.763ms
[2.288s][info][gc] GC(4) Pause Young (Normal) (G1 Evacuation Pause) 
23M->12M(40M) 6.382ms
[2.444s][info][gc] GC(5) Pause Young (Concurrent Start) (Metadata 
GC Threshold) 18M->12M(48M) 7.579ms

[2.444s][info][gc] GC(6) Concurrent Cycle
[2.481s][info][gc] GC(6) Pause Remark 13M->13M(48M) 5.255ms
[2.498s][info][gc] GC(6) Pause Cleanup 13M->13M(48M) 0.090ms
[2.499s][info][gc] GC(6) Concurrent Cycle 54.811ms
[2.905s][info][gc] GC(7) Pause Young (Normal) (G1 Evacuation Pause) 
26M->13M(48M) 12.726ms
[3.204s][info][gc] GC(8) Pause Young (Normal) (GCLocker Initiated 
GC) 29M->15M(48M) 11.216ms
[3.462s][info][gc] GC(9) Pause Young (Normal) (G1 Evacuation Pause) 
30M->17M(48M) 18.043ms
[3.679s][info][gc] GC(10) Pause Young (Normal) (G1 Evacuation 
Pause) 31M->18M(64M) 15.195ms
[3.933s][info][gc] GC(11) Pause Young (Normal) (G1 Evacuation 
Pause) 38M->20M(64M) 9.412ms
[4.230s][info][gc] GC(12) Pause Young (Normal) (G1 Evacuation 
Pause) 40M->21M(64M) 16.319ms
[4.536s][info][gc] GC(13) Pause Young (Normal) (G1 Evacuation 
Pause) 41M->23M(64M) 23.897ms
[4.750s][info][gc] GC(14) Pause Young (Normal) (G1 Evacuation 
Pause) 43M->24M(94M) 8.776ms
[5.180s][info][gc] GC(15) Pause Young (Normal) (G1 Evacuation 
Pause) 58M->26M(94M) 15.610ms
[5.546s][info][gc] GC(16) Pause Young (Normal) (G1 Evacuation 
Pause) 67M->27M(94M) 18.075ms
[6.058s][info][gc] GC(17) Pause Young (Normal) (G1 Evacuation 
Pause) 69M->30M(94M) 32.625ms
[7.268s][info][gc] GC(18) Pause Young (Normal) (G1 Evacuation 
Pause) 71M->31M(156M) 18.999ms
[7.458s][info][gc] GC(19) Pause Young (Concurrent Start) (Metadata 
GC Threshold) 40M->31M(156M) 20.217ms

[7.459s][info][gc] GC(20) Concurrent Cycle
[7.676s][info][gc] GC(20) Pause Remark 35M->35M(156M) 19.304ms
[7.748s][info][gc] GC(20) Pause Cleanup 36M->36M(156M) 0.183ms
[7.782s][info][gc] 

Re: RFR: 8207851 JEP Draft: Support ByteBuffer mapped over non-volatile memory

[Brian Goetz]

> Hi Alan/Brian,

My apologies for having missed this the first time around; messages to lists 
get swept into folders, and staying on top of many lists is not an exact 
science.  


> I believe I have addressed all outstanding comments on the JEP per se,
> including those made by Alan. Is it now possible for one of you to
> endorse the JEP so it can be submitted?

You don’t actually need my endorsement to Submit a JEP to be a Candidate; all 
you need is evidence that it’s been socialized on the right list (check) and a 
reviewer (check).  So you’re not blocked on submitting at all.  

That said, Candidate is actually supposed to be an early stage in the 
lifecycle; making something a candidate is essentially a statement that the 
feature is worth considering.  It is not any sort of commitment that the 
feature meets the high bar for inclusion, or is done — just that it is 
something that is worth continuing to work on in OpenJDK.  

Clearly, you’ve done a lot more than the minimum to meet the Candidate bar.  
Where I see this JEP at is you’ve iterated on the implementation and the design 
for a few rounds — and where we’re at now is we’re facing the stewardship 
questions of exactly what the right way to position this feature in the JDK is. 
 Based on the list discussion so far, it seems like we’ve not yet reached 
consensus on this, so these discussions can continue, whether the JEP is a 
Candidate or not.  

I concur with Alan and Stuart’s assessment that this is very much a “stop gap” 
measure.  (That’s not a value judgment, either the design of MBB, or your 
work.)  It is clear that MBBs are reaching the end of their design lifetime, 
and the set of mismatches to the problems people want to solve are growing, and 
the amount of investment needed to bring MBBs up to the task would be 
significant.  It seems likely a much better long-term solution is Panama, but I 
agree that Panama is not yet ready to replace MBB, and so some stop-gap is 
needed to prolong the usefulness of MBBs for long enough for a better solution 
to emerge.  However, Alan’s concern — which I share — is that by expanding the 
MBB API now, we open ourselves up to increased maintenance costs in the 
too-near future.  Before we settle on an API, we’re going to need to come to 
consensus on the right tradeoffs here, and I’m not convinced we’ve found this 
yet.  So we’ll keep talking.  

> Is there any other impediment to submitting the JEP and proceeding to
> code review?

There do not seem to be impediments to submitting the JEP, but I would call out 
the assumption that the next stop is code review, and then integration.  That 
seems to want to skip over the far more important “should we / how should we” 
discussions, which are still in progress.  

Cheers,
-Brian

Re: RFR: 8207851 JEP Draft: Support ByteBuffer mapped over non-volatile memory

[Andrew Dinn]

Hi Alan,

On 18/01/2019 13:32, Alan Bateman wrote:

> I had a brief discussion with Brian about this yesterday. He brought up
> the same concern about using MBB as it's not the right API for this in
> the longer term.  So this JEP is very much about a short term/tactical
> solution as we've already concluded here. This leads to the question as
> to whether this JEP needs to evolve the standard/Java SE API or not.
> It's convenient for the implementation of course but we should at least
> explore doing this as a JDK-specific feature.

I disagree with your characterization of use of MBB as a short term/
tactical solution. Despite not being entirely suitable for the task MBB
is a de facto standard way for many applications to gain direct access
to files of data located on persistent storage. The current proposal is
not, as you characterize it, a quick fix to use MBB as a temporary way
to access NVM storage until something better comes along. The intention
is rather to ensure that the current API caters for a new addition to
the persistent memory tier. The imperative is to allow existing code to
employ it now.

Of course, a better API may come along for accessing persistent storage,
whether that be NVM, flash disk or spinning platter. However, I would
hazard that in many cases existing application code and libraries will
still want/need to continue to use the MBB API, including cases where
that storage can most usefully be NVM. Rewriting application code to use
a new API will not always be feasible or cost-effective. Yet, the
improved speed of NVM suggests that an API encompassing this new case
will be very welcome and may well be cost-effective to adopt.

In sum, far from being a stop-gap this proposal should be seen as a step
towards completing and maintaining the existing MBB API for emergent tech.

> To that end, one approach to explore is allowing the FC.map method
> accept map modes beyond those defined by MapMode. There is precedence
> for extensibility in this area already, e.g. FC.open allows you to
> specify options beyond the standard options specified by the method. It
> would require MapMode to define a protected constructor and would
> require a bit of plumbing to support MapMode defined in a JDK-specific
> module but there are examples to point to. Another approach is aanother
> class in a JDK-specific module to define the map method. It would
> require the same plumbing under the covers but would avoid touch the FC
> spec.
I'm not sure what this side-step is supposed to achieve nor how that
relates to the concerns over use of MBB (perhaps it doesn't). I'm not
really clear what problem you are trying to avoid here by allowing the
MapMode enum to be extensible via a protected constructor.

If your desire is to avoid adding extra API surface to FileChannel then
where would you consider it appropriate to add such a surface. Something
is going to have to create and employ the extra enum tags that are
currently proposed for addition to MapMode. How is a client application
going to reach that something?

Perhaps we might benefit form looking at a simple example? Currently, my
most basic test program drives the API to create an MBB as follows:

. . .
String dir = "/mnt/pmem/test"; // mapSync should work, since fs
mount is -o dax

Path path = new File(dir, "pmemtest").toPath();

FileChannel fileChannel = (FileChannel) Files
.newByteChannel(path, EnumSet.of(
StandardOpenOption.READ,
StandardOpenOption.WRITE,
StandardOpenOption.CREATE));

MappedByteBuffer mappedByteBuffer =
fileChannel.map(FileChannel.MapMode.READ_WRITE_PERSISTENT, 0, 1024);
. . .

Could you give a sketch of an alternative way that you see a client
operating?

One thing I did wonder about was whether we could insert the relevant
behavioural switch in the call to Files.newByteChannel rather than the
map call?

If we passed an ExtendedOpenOption (e.g. ExtendedOpenOption.SYNC) to
this call then method newByteChannel could create and return a
corresponding variant of FleChannelImpl, say an instance of a subclass
called SyncFileChannelImpl. This could behave as per a normal
FileChannelImpl apart from adding the MAP_SYNC flag to the mmap call
(well, also rejecting PRIVATE maps).

Would that be a better way to drive this? Would it address the concerns
you raised above?

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Alan Bateman]

On 21/01/2019 09:17, Langer, Christoph wrote:

:
OK, I can see the point that in a PosixFileAttributeView as it is, there's no 
place for optionality/null values. However, with this approach the benefits 
would be that Files::get/setPosixPermissions would work and that's why I think 
we should pursue this. The challenge will be to find reasonable defaults.
That is how extensions are suppose to work. I think Sherman has looked 
at exporting zipfs specific FileAttributeViews a couple of times. Yes, 
it doesn't mean compiling against a JDK-specific API but we have several 
JDK specific modules that export APIs.



:

I can imagine something like this:
Zipfs by default implements an own view that offers dynamic, not type safe access to 
"zip:permissions" and we'll document this. If a user of zipfs wants to see full 
PosixFileAttributeView support with default values, then we should allow for a creation 
attribute for the zipfs that can control this. Maybe we can even allow specifying default 
values for user, group and permissions via zipfs attributes.
The configuration parameters that you specify to newFileSystem can work 
like mount options so provide some way to specify defaults may be 
useful. When you mount a fat32 file system on a Linux/Unix system then 
you can usually configure things like the umask which may give you ideas.


-Alan

Re: RFR: JDK-8217269: jpackage Makefile improvments

[Magnus Ihse Bursie]

On 2019-01-18 15:18, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


The webrev includes all the jpackage Makefile Improvements listed in 
[1], other than what is covered by [4], and also includes fix for 
white space errors requested in [3].


Looks good to me! Thanks for fixing this.

In the future, please cc build-dev for build related fixes.

/Magnus



[1] https://bugs.openjdk.java.net/browse/JDK-8217269

[2] http://cr.openjdk.java.net/~herrick/8217269/webrev.01/

[3] https://bugs.openjdk.java.net/browse/JDK-8216521

[4] https://bugs.openjdk.java.net/browse/JDK-8217317

/Andy

RE: RFR(XS): 8217438: Adapt tools//launcher/Test7029048.java for AIX

[Langer, Christoph]

Hi Goetz,

looks good to me. I think it should rather be "AIX" than "Aix" in the output 
text - but it's a developer trace only, so probably nobody cares. 

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Lindenmaier, Goetz
> Sent: Montag, 21. Januar 2019 11:01
> To: core-libs-dev@openjdk.java.net
> Subject: [CAUTION] RFR(XS): 8217438: Adapt
> tools//launcher/Test7029048.java for AIX
> 
> Hi,
> 
> please review the following small test fix:
> http://cr.openjdk.java.net/~goetz/wr19/8217438-aix_launcherTest/01/
> 
> The Aix launcher always sets the path to the jdk libraries because
> Aix does not support $ORIGIN to find libjvm.so.
> So some of the test cases here fail. Skip them.
> 
> Best regards,
>   Goetz.

Re: [13] RFR: 8210583: Base64.Encoder incorrectly throws NegativeArraySizeException

[Nishit Jain]

Hi Joe,

Executing it on Mach5 platforms took around 3s to 5s. Below is the link

https://java.se.oracle.com:10065/mdash/jobs/nishjain-jdk_04_09-20190121-0805-19402/results?search=status%3A*

Also executed using JMH on local linux-64 VM (6GB RAM), it took ~2200 ms 
in SingleShotTime mode


Benchmark   Mode  Cnt Score  Error  Units
MyBenchmark.testMethod    ss   50  2200.663 ± 1826.348  ms/op

Regards,
Nishit Jain
On 19-01-2019 06:09, Joe Darcy wrote:
How long does the regression test take to run on machines that have 
enough memory?


Thanks,

-Joe

On 1/18/2019 3:38 PM, naoto.s...@oracle.com wrote:

Looks good, Nishit.

Naoto

On 1/18/19 3:03 AM, Nishit Jain wrote:

Hi Roger, Naoto,

 > The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.

When Integer.MAX_VALUE - 8 is used, the decode methods do not throw 
the added OOME, because the computed value does not overflow 
Integer.MAX_VALUE, so to test the added OOME condition need to use 
Integer.MAX_VALUE - 2.


Updated the webrev with rest of the suggestions

http://cr.openjdk.java.net/~nishjain/8210583/webrev.04/

Regards,
Nishit Jain
On 18-01-2019 02:44, Naoto Sato wrote:
I agree with 'withOutputParam' comment. It does seem to require 
some explanation. Same for the newly introduced return value -1.


The test:
  46 // A separate output array is not required, as it is 
not used and
  47 // the exception is thrown beforehand while 
calculating the size

  48 // of the encoded bytes using input array

This seems to make an assumption on the implementation. Test should 
not depend on the internal impl.


Naoto

On 1/17/19 8:14 AM, Roger Riggs wrote:

Hi Nishit,

In the test, there are a couple of RuntimeExceptions with messages 
that start with "As expected"...


That's counter intuitive, that a failure of the test is reported 
with a message saying, its normal!

I would use a message like:  "XXException should have been thrown..."

The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.

java/util/Base64.java:

Lines 335-342:  This optimization is not mentioned in the bug 
report and

should be a separate review.


245, 686:  in outLength(), the 2nd parameter would be easier to 
understand

as 'throwOOME', meaning to throw OOM if length is too large.
The 'withOutputParam' only has any meaning in the context of the 
caller.
And even for the private method write the javadoc describing the 
behavior.


It also makes the call sites clearer, the argument will be true to 
throw.


Thanks, Roger


On 01/17/2019 02:07 AM, Nishit Jain wrote:

Hi,

Please review the fix for 8210583

Bug: https://bugs.openjdk.java.net/browse/JDK-8210583
Webrev: http://cr.openjdk.java.net/~nishjain/8210583/webrev.03/
CSR: https://bugs.openjdk.java.net/browse/JDK-8215633

Issue: Base64.Encoder.encode and Base64.Decoder.decode methods 
incorrectly throw unspecified exception e.g. 
NegativeArraySizeException, when the input byte array size is too 
large such that the calculated output byte size goes beyond the 
max-integer boundary and wraps around.


Fix: Throw an OutOfMemoryError if the output byte array/buffer or 
memory can not be allocated. There is an unrelated change in 
encodeToString(byte[]) where a string instance is created using 
JavaLangAccess.newStringNoRepl(byte[], ISO_8859_1)instead of 
string constructor, to save memory.


Regards,
Nishit Jain

Re: [13] RFR: 8210583: Base64.Encoder incorrectly throws NegativeArraySizeException

[Nishit Jain]

Thanks Roger,

Updated.
http://cr.openjdk.java.net/~nishjain/8210583/webrev.05/

Regards,
Nishit Jain
On 18-01-2019 21:13, Roger Riggs wrote:

Hi Nishit,

Looks good, with a minor fix.

ok, the rationale for MAX_VALUE-2 make sense.

TestEncodingDecodingLength: Line 61 and 68,
  The error message will be more readable with a ": " before the 
methodname is appended.


Thanks, Roger


On 01/18/2019 06:03 AM, Nishit Jain wrote:

Hi Roger, Naoto,

> The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.

When Integer.MAX_VALUE - 8 is used, the decode methods do not throw 
the added OOME, because the computed value does not overflow 
Integer.MAX_VALUE, so to test the added OOME condition need to use 
Integer.MAX_VALUE - 2.


Updated the webrev with rest of the suggestions

http://cr.openjdk.java.net/~nishjain/8210583/webrev.04/

Regards,
Nishit Jain
On 18-01-2019 02:44, Naoto Sato wrote:
I agree with 'withOutputParam' comment. It does seem to require some 
explanation. Same for the newly introduced return value -1.


The test:
  46 // A separate output array is not required, as it is 
not used and
  47 // the exception is thrown beforehand while calculating 
the size

  48 // of the encoded bytes using input array

This seems to make an assumption on the implementation. Test should 
not depend on the internal impl.


Naoto

On 1/17/19 8:14 AM, Roger Riggs wrote:

Hi Nishit,

In the test, there are a couple of RuntimeExceptions with messages 
that start with "As expected"...


That's counter intuitive, that a failure of the test is reported 
with a message saying, its normal!

I would use a message like:  "XXException should have been thrown..."

The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.

java/util/Base64.java:

Lines 335-342:  This optimization is not mentioned in the bug 
report and

should be a separate review.


245, 686:  in outLength(), the 2nd parameter would be easier to 
understand

as 'throwOOME', meaning to throw OOM if length is too large.
The 'withOutputParam' only has any meaning in the context of the 
caller.
And even for the private method write the javadoc describing the 
behavior.


It also makes the call sites clearer, the argument will be true to 
throw.


Thanks, Roger


On 01/17/2019 02:07 AM, Nishit Jain wrote:

Hi,

Please review the fix for 8210583

Bug: https://bugs.openjdk.java.net/browse/JDK-8210583
Webrev: http://cr.openjdk.java.net/~nishjain/8210583/webrev.03/
CSR: https://bugs.openjdk.java.net/browse/JDK-8215633

Issue: Base64.Encoder.encode and Base64.Decoder.decode methods 
incorrectly throw unspecified exception e.g. 
NegativeArraySizeException, when the input byte array size is too 
large such that the calculated output byte size goes beyond the 
max-integer boundary and wraps around.


Fix: Throw an OutOfMemoryError if the output byte array/buffer or 
memory can not be allocated. There is an unrelated change in 
encodeToString(byte[]) where a string instance is created using 
JavaLangAccess.newStringNoRepl(byte[], ISO_8859_1)instead of 
string constructor, to save memory.


Regards,
Nishit Jain

RFR(XS): 8217438: Adapt tools//launcher/Test7029048.java for AIX

[Lindenmaier, Goetz]

Hi,

please review the following small test fix:
http://cr.openjdk.java.net/~goetz/wr19/8217438-aix_launcherTest/01/

The Aix launcher always sets the path to the jdk libraries because
Aix does not support $ORIGIN to find libjvm.so.
So some of the test cases here fail. Skip them.

Best regards,
  Goetz.

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Alan,

first of all, thank you for your input on this.

> I think the approach to explore are:
> 
> 1. zipfs supports PosixFileAttributeView without subsetting. If
> readAttribute(file, BasicFileAttributes.class) succeeds then
> readAttribute(file, PosixFileAttributes.class) should also succeed, even
> if there aren't permissions encoded in the zip entry's external file
> attributes. It would mean that owner and group return default values,
> and permissions may return a default value. It does mean you can't
> distinguish the default value from "no permissions" but there is
> precedence for that, e.g. mount a FAT32 file system on Linux or Unix
> systems and `stat` a file to have the stat structure populated with
> default uid, gid and mode bits.

OK, I can see the point that in a PosixFileAttributeView as it is, there's no 
place for optionality/null values. However, with this approach the benefits 
would be that Files::get/setPosixPermissions would work and that's why I think 
we should pursue this. The challenge will be to find reasonable defaults.

> 2. zipfs defines a new FileAttributeView that defines read and write
> access to permissions stored in a zip entry's external file attribute.
> As it's a new view then it can define the behavior for the case that the
> zip entry doesn't have permissions. Furthermore it does not need to
> extend BasicFileAttributeView so doesn't need to be concerned with bulk
> access, nor concerned with group/owner. As you know, the attributes API
> allows for both type safe and dynamic access so you have a choice as to
> whether to support both or just dynamic access. With the first then
> jdk.zipfs would export a package with a public interface that defines
> the view. If someone wants type safe access to the permissions attribute
> then you need to import the class. The alternative is to not export any
> packages but just define the view in the module-info. The view its name
> and define the name/type of the permissions attribute, it will also
> define how it behaves when the external attributes aren't populated. In
> usage terms it means reading the permissions will be something like
> Files.readAttribute(file, "zip:permissions") and casting the value to
> Set - not pretty but it avoids depending on a
> JDK-specific API.

For this approach, there are 2 things I dislike. The first is that I don't 
think we should export named packages from module jdk.zipfs that people would 
develop Java code against while not being in the Java API. And secondly, this 
way would not support using Files::set/getPosixPermissions since the 
specification/implementation of that utility method explicitly refers to 
PosixFileAttributeView.

I can imagine something like this:
Zipfs by default implements an own view that offers dynamic, not type safe 
access to "zip:permissions" and we'll document this. If a user of zipfs wants 
to see full PosixFileAttributeView support with default values, then we should 
allow for a creation attribute for the zipfs that can control this. Maybe we 
can even allow specifying default values for user, group and permissions via 
zipfs attributes.

I'll work to develop the patch into this direction unless you tell me that this 
idea is bogus (if so, then I hope it be soon )

Thanks
Christoph