Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Ivan Gerasimov]

On 7/10/19 5:17 PM, Brian Burkhalter wrote:

I incorporated Peter’s version, adding the security check in 
cancelDeleteOnExit(), tweaking its verbiage along with that of deleteOnExit(), 
and modified the test DeleteOnExit to verify the new method. The updated 
version is here:

http://cr.openjdk.java.net/~bpb/8193072/webrev.03/ 


There is possibility of a race here in a scenario like this:

File dir = new File("dir");
File file = new File("dir/file");

-- thread 1 --
dir.deleteOnExit();
file.deleteOnExit();
///
dir.cancelDeleteOnExit();
  thread 2 intervenes
file.cancelDeleteOnExit();
-- end --

-- thread 2 --
dir.deleteOnExit();
file.deleteOnExit();
-- end --

The result will be that the order of the registered files will change, 
and JVM will try to delete dir first (this will fail as it is not empty).


Of course it could be avoided, if cancellation were done in reverse 
order, though it's not immediately obvious from the documentation.


With kind regards,
Ivan

Thanks,

Brian


On Jul 10, 2019, at 11:17 AM, Brian Burkhalter  
wrote:

On Jul 10, 2019, at 5:36 AM, Peter Levart  wrote:

There are various interleavings of threads that could cause the file to be left 
undeleted when VM exits.

To cover concurrent scenarios such as above, the code could use reference 
counting. Like in the following patch:

http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/ 


This looks good to me modulo adding this
SecurityManager security = System.getSecurityManager();
if (security != null) {
security.checkDelete(path);
}
to cancelDeleteOnExit() as suggested below.


On Jul 10, 2019, at 7:51 AM, Sean Mullan  wrote:

On 7/9/19 7:40 PM, Brian Burkhalter wrote:

I don’t know. On the one hand this does not take an action like reading, 
writing, or deleting, but on the other it could end up causing files to be left 
lying around after VM termination which were expected to be deleted. I suppose 
that could be considered to be some sort of security issue.

Yes I think so.

I would probably just use the same permission (FilePermission(file,"delete")). 
If you have been granted permission to delete a file, then you should have permission to 
cancel that deletion as well.

That’s a  good idea.


--
With kind regards,
Ivan Gerasimov

Re: RFR [14/java.xml] 8178843: A bug in an inner loop in MethodGenerator's getLocals method

[Lance Andersen]

+1
> On Jul 10, 2019, at 7:57 PM, Joe Wang  wrote:
> 
> Please review a cleanup that removes unused code. This code has two errors, 
> one as indicated in the bug report, another attempting to loop through 
> allVarsEverDeclared that was just created (meaning size=0). It just ensures 
> again that the code was never used.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8178843
> webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8178843/webrev/
> 
> All tests passed.
> 
> Thanks,
> Joe

 
  

 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: EnumSet.class serialization broken - twice - JDK-8227368

[Stuart Marks]

On 7/10/19 9:54 AM, Peter Levart wrote:

On 7/10/19 6:03 PM, Joe Darcy wrote:

Hello,

I'd advise against including at least part of the comment

    // declare serialization compatibility with JDK 8 (see JDK-8227368)

We generally don't include bug numbers in the text of the code and rely 
mapping from the SCM to the bug database to provide the (sometimes large!) 
additional context for a fix.


Ok, Joe. I removed the reference to bug number and recreated webrev.03 in-place. 
Will that be OK?


http://cr.openjdk.java.net/~plevart/jdk-dev/8227368_EnumSet.class_serialization/webrev.03/ 


Hi Peter,

I'm ok with this patch (i.e., without the bug id). I did a test run and it looks 
good. Given that Joe approved the CSR, you're now cleared to push. Remember to 
push to JDK 13!


We should probably discuss the JDK 11 backport on jdk-updates, since it'll 
differ from the JDK 13 patch.


Joe,

For this patch having the bug id doesn't matter very much, since there is 
nothing unusual about this code as it stands, and it'll be ok for the long term.


However, there are a bunch of places in the source code where we do reference 
bug reports; they're pretty easy to find. There are perhaps over 100 in 
java.base alone. (Some include full URLs, which I think is unnecessary.)


I think it's easier to go directly to a bug report than to search through the 
history for bug ids in changeset comments. "hg anno" works reasonably well 
initially, but as time goes on the changeset associated with a particular line 
of code can be obscured by incidental refactorings, file renames, etc.


For example, I expect the JDK 11 backport to assign the serialVersionUID in a 
static initializer block (which will require warnings suppression as well). This 
is unusual and warrants a comment. This could either be a complete explanation 
(which might run to a paragraph or more) or a reference to a bug id. Sometimes 
an in-line explanation is warranted, but there's enough history here that in 
this case a reference to a bug id seems more appropriate.


s'marks

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

I incorporated Peter’s version, adding the security check in 
cancelDeleteOnExit(), tweaking its verbiage along with that of deleteOnExit(), 
and modified the test DeleteOnExit to verify the new method. The updated 
version is here:

http://cr.openjdk.java.net/~bpb/8193072/webrev.03/ 


Thanks,

Brian

> On Jul 10, 2019, at 11:17 AM, Brian Burkhalter  
> wrote:
> 
> On Jul 10, 2019, at 5:36 AM, Peter Levart  wrote:
>> 
>> There are various interleavings of threads that could cause the file to be 
>> left undeleted when VM exits.
>> 
>> To cover concurrent scenarios such as above, the code could use reference 
>> counting. Like in the following patch:
>> 
>> http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/
>>  
>> 
> This looks good to me modulo adding this
>SecurityManager security = System.getSecurityManager();
>if (security != null) {
>security.checkDelete(path);
>}
> to cancelDeleteOnExit() as suggested below.
> 
>> On Jul 10, 2019, at 7:51 AM, Sean Mullan  wrote:
>> 
>> On 7/9/19 7:40 PM, Brian Burkhalter wrote:
>>> I don’t know. On the one hand this does not take an action like reading, 
>>> writing, or deleting, but on the other it could end up causing files to be 
>>> left lying around after VM termination which were expected to be deleted. I 
>>> suppose that could be considered to be some sort of security issue.
>> 
>> Yes I think so.
>> 
>> I would probably just use the same permission 
>> (FilePermission(file,"delete")). If you have been granted permission to 
>> delete a file, then you should have permission to cancel that deletion as 
>> well.
> 
> That’s a  good idea.

RFR [14/java.xml] 8178843: A bug in an inner loop in MethodGenerator's getLocals method

[Joe Wang]

Please review a cleanup that removes unused code. This code has two 
errors, one as indicated in the bug report, another attempting to loop 
through allVarsEverDeclared that was just created (meaning size=0). It 
just ensures again that the code was never used.


JBS: https://bugs.openjdk.java.net/browse/JDK-8178843
webrevs: http://cr.openjdk.java.net/~joehw/jdk14/8178843/webrev/

All tests passed.

Thanks,
Joe

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

Peter / Sean,

Thanks for the comments.

> On Jul 10, 2019, at 5:36 AM, Peter Levart  wrote:
> 
> There are various interleavings of threads that could cause the file to be 
> left undeleted when VM exits.
> 
> To cover concurrent scenarios such as above, the code could use reference 
> counting. Like in the following patch:
> 
> http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/
>  
> 
This looks good to me modulo adding this
SecurityManager security = System.getSecurityManager();
if (security != null) {
security.checkDelete(path);
}
to cancelDeleteOnExit() as suggested below.

> On Jul 10, 2019, at 7:51 AM, Sean Mullan  wrote:
> 
> On 7/9/19 7:40 PM, Brian Burkhalter wrote:
>> I don’t know. On the one hand this does not take an action like reading, 
>> writing, or deleting, but on the other it could end up causing files to be 
>> left lying around after VM termination which were expected to be deleted. I 
>> suppose that could be considered to be some sort of security issue.
> 
> Yes I think so.
> 
> I would probably just use the same permission 
> (FilePermission(file,"delete")). If you have been granted permission to 
> delete a file, then you should have permission to cancel that deletion as 
> well.

That’s a  good idea.

Thanks,

Brian

Re: EnumSet.class serialization broken - twice - JDK-8227368

[Peter Levart]

On 7/10/19 6:03 PM, Joe Darcy wrote:

Hello,

I'd advise against including at least part of the comment

    // declare serialization compatibility with JDK 8 (see JDK-8227368)

We generally don't include bug numbers in the text of the code and 
rely mapping from the SCM to the bug database to provide the 
(sometimes large!) additional context for a fix.


Ok, Joe. I removed the reference to bug number and recreated webrev.03 
in-place. Will that be OK?


http://cr.openjdk.java.net/~plevart/jdk-dev/8227368_EnumSet.class_serialization/webrev.03/

Regards, Peter

Re: [14]RFR of JDK-8227289:Enable assertions for some shell to java conversion tests after JDK-8218960

[naoto . sato]

Looks good to me.

Naoto

On 7/9/19 11:53 PM, Ying Zhou wrote:

Hello,

Please review this patch for updating below tests by adding -ea/esa to 
launcher parameters.


- /test/jdk/java/util/Calendar/SupplementalJapaneseEraTestRun.java
- /test/jdk/java/util/TimeZone/TimeZoneDatePermissionCheckRun.java
- /test/jdk/java/util/TimeZone/Bug8066652Run.java
- 
/test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java

- /test/jdk/java/util/ResourceBundle/modules/ModuleTestUtil.java
- /test/jdk/java/util/ResourceBundle/modules/layer/LayerTest.java
- /test/jdk/java/util/ResourceBundle/modules/unnamed/UnNamedTest.java
- /test/jdk/java/util/ResourceBundle/modules/visibility/VisibilityTest.java

JDK Bug: JDK-8227289 

webrev: http://cr.openjdk.java.net/~yzhou/8227289/webrev.00/

Thanks,

Daisy

Re: EnumSet.class serialization broken - twice - JDK-8227368

[Joe Darcy]

Hello,

I'd advise against including at least part of the comment

    // declare serialization compatibility with JDK 8 (see JDK-8227368)

We generally don't include bug numbers in the text of the code and rely 
mapping from the SCM to the bug database to provide the (sometimes 
large!) additional context for a fix.


Cheers,

-Joe

On 7/10/2019 4:32 AM, Peter Levart wrote:

Hi Stuart,

On 7/10/19 4:32 AM, Stuart Marks wrote:

1. New changeset with constant version of EnumSet.serialVersionUID.


This is already in the webrev.01 changeset. webrev.02 is an attempt 
to sneak the change without being visible in the serialized-form.html.


Ah. I skipped webrev.01 because I thought that webrev.02 had 
superseded it. Looking at webrev.01, I see



+    // value computed from JDK 8 (and previous) EnumSet class
+    // needed to properly cross-(de)serialize EnumSet.class objects
+    // between JDK 8- <-> JDK 9+
+    private static final long serialVersionUID = 1009687484059888093L;
+


I don't think this comment can cover the entire history here. We'll 
have to rely on the bug report, the CSR, and the email archives.


Most declarations of serialVersionUID don't have a comment at all. 
So, we could just omit it.


If you feel a comment is necessary, perhaps something like

// declare serialization compatibility with JDK 8 (see JDK-8227368)


I created webrev.03 with your proposed comment:

http://cr.openjdk.java.net/~plevart/jdk-dev/8227368_EnumSet.class_serialization/webrev.03/ 





might be sufficient. Otherwise, webrev.01 looks fine.


On 7/9/19 1:57 AM, Stuart Marks wrote:
2. Create draft CSR. 


Created:

https://bugs.openjdk.java.net/browse/JDK-8227432


I've done some editing on this CSR and I've marked myself as a 
reviewer. Please move this to Finalized.


Thank you for your edits. It reads much better now :-) I have 
Finalized it.




While we're waiting for the CSR to be approved (I hope this takes 
only a day or two) I'll do some testing with your patch.


Thank you for testing.

Regards, Peter

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Sean Mullan]

On 7/9/19 7:40 PM, Brian Burkhalter wrote:

I don’t know. On the one hand this does not take an action like reading, 
writing, or deleting, but on the other it could end up causing files to be left 
lying around after VM termination which were expected to be deleted. I suppose 
that could be considered to be some sort of security issue.


Yes I think so.

I would probably just use the same permission 
(FilePermission(file,"delete")). If you have been granted permission to 
delete a file, then you should have permission to cancel that deletion 
as well.


--Sean




Thanks,

Brian


On Jul 9, 2019, at 2:26 PM, Jason Mehrens  wrote:

Would the SecurityManager need to for permissions (checkWrite or some new 
permission) before cancelDeleteOnExit() is allowed?

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Peter Levart]

Hi,

On 7/9/19 8:08 PM, Brian Burkhalter wrote:

On Jul 9, 2019, at 8:31 AM, Brian Burkhalter  
wrote:


Since deleteOnExit() is an deliberate act, perhaps there should be a 
corresponding withdrawDeleteOnExit() that reverses it so that it is intentional 
and not a side-effect of other File methods.

I think this is a better idea. Perhaps “cancelDeleteOnExit()”.

If we want to go this route, here is one possibility:

http://cr.openjdk.java.net/~bpb/8193072/webrev.02/




With only one method (deleteOnExit) there are no races because the 
method is idempotent if called with the same File multiple times from 
different threads. Adding cancelDeleteOnExit() makes things problematic 
in concurrent setting. Imagine the following code:


    File f = new File("/path/to/file");

        // 1st register hook...
    f.deleteOnExit();
    // ...then attempt file creation so there's no chance
    // the file is left behind if VM unexpectedly exits
    if (f.createNewFile()) {
    ...
            ... process something using f
    ...
    // 1st delete file...
    f.delete();
    }
    // ...then unregister hook so there's no chance
    // the file is left behind if VM unexpectedly exits
    // unregister hook also after we registered it but
    // then file creation failed.
    f.undoDeleteOnExit();

This code is correct if executed in a single thread. But imagine two or 
more threads competing to create the same file and properly delete it 
afterwards with registering and un-registering the hook to cover cleanup 
when VM exits during processing...


There are various interleavings of threads that could cause the file to 
be left undeleted when VM exits.


To cover concurrent scenarios such as above, the code could use 
reference counting. Like in the following patch:


http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/

Regards, Peter

Re: EnumSet.class serialization broken - twice - JDK-8227368

[Peter Levart]

Hi Stuart,

On 7/10/19 4:32 AM, Stuart Marks wrote:

1. New changeset with constant version of EnumSet.serialVersionUID.


This is already in the webrev.01 changeset. webrev.02 is an attempt 
to sneak the change without being visible in the serialized-form.html.


Ah. I skipped webrev.01 because I thought that webrev.02 had 
superseded it. Looking at webrev.01, I see



+    // value computed from JDK 8 (and previous) EnumSet class
+    // needed to properly cross-(de)serialize EnumSet.class objects
+    // between JDK 8- <-> JDK 9+
+    private static final long serialVersionUID = 1009687484059888093L;
+


I don't think this comment can cover the entire history here. We'll 
have to rely on the bug report, the CSR, and the email archives.


Most declarations of serialVersionUID don't have a comment at all. So, 
we could just omit it.


If you feel a comment is necessary, perhaps something like

// declare serialization compatibility with JDK 8 (see JDK-8227368)


I created webrev.03 with your proposed comment:

http://cr.openjdk.java.net/~plevart/jdk-dev/8227368_EnumSet.class_serialization/webrev.03/



might be sufficient. Otherwise, webrev.01 looks fine.


On 7/9/19 1:57 AM, Stuart Marks wrote:
2. Create draft CSR. 


Created:

https://bugs.openjdk.java.net/browse/JDK-8227432


I've done some editing on this CSR and I've marked myself as a 
reviewer. Please move this to Finalized.


Thank you for your edits. It reads much better now :-) I have Finalized it.



While we're waiting for the CSR to be approved (I hope this takes only 
a day or two) I'll do some testing with your patch.


Thank you for testing.

Regards, Peter

Re: RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

On 09/07/19 8:44 PM, David Lloyd wrote:
> On Tue, Jul 9, 2019 at 8:19 AM Jaikiran Pai  wrote:
>> - In addition, I have sneaked in an unrelated change in this patch, into
>> the Deflater.end() method:
>>
>>  public void end() {
>>  synchronized (zsRef) {
>> +// check if already closed
>> +if (zsRef.address() == 0) {
>> +return;
>> +}
>>  zsRef.clean();
>>  input = ZipUtils.defaultBuf;
>> +inputArray = null;
>> +}
>>
>> Unlike in the Inflater.end(), the Deflater.end() didn't previously have
>> the "inputArray = null". I have included it in this patch, since it
>> looks right to clean up that array too. If this isn't the right
>> time/patch to do it, please do let me know and I'll take that up separately.
> This was probably my mistake.  The fix looks good to me!


Thank you David.

-Jaikiran

Re: RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Lance,

On 10/07/19 2:25 AM, Lance Andersen wrote:
> ———
> @implSpec This method is a no-op if this compressor has already
> 886 * been previously closed,
>
> 
>
> Please remove “already” in both the close() and end() methods.

Done.


>  I believe the preference is the @implSpec and its relatives are on
> their own line as in https://openjdk.java.net/jeps/8068562 and was
> done for @apiNote earlier

Done.


>
> Outside of the @ImplSpec, I am not sure the initial wording for end()
> and close() really need to differ:
>
> end():
>
> Closes the decompressor and discards any unprocessed input.
>
> close():
>
> Releases resources held by this decompressor and discards any
> unprocessed input.This method should be called when the
> decompressor is no longer needed
>
>
I have now updated the javadoc of end() to be closer to the javadoc of
close().


>
>> - The javadoc of end() method in both these classes has been updated to
>> encourage the use of close() method instead of this one. It now also has
>> a @implSpec which states that it's a no-op if called more than once.
>>
>> In addition, this javadoc has also been updated to replace the
>> "undefined behaviour" statement with a mention that the usage of the
>> Inflater/Deflater instance after a call to end() may throw an exception
>> in those subsequent usages. Please note that, there's no such explicit
>> mention in the javadoc of the (newly added) close() method because IMO,
>> it isn't needed for close() since I think it's kind of implied that a
>> closed resource can no longer be used for further operations.
>
> We need to be specific in close()  also for clarity

I haven't updated this in the latest webrev version and will wait for us
to come to a decision on how we word it for end().


>>
>>
>> - TotalInOut.java test has been updated to use the new
>> try-with-resources construct for the inflater and deflater it uses.
>
> Please update @biug to include the bug number

Done.


>>
>> - A couple of new (testng) test classes have been added to test various
>> scenarios where close() and end() method get called. These test mostly
>> focus on ensuring that the close() and end(), either implicitly or
>> explicitly, get called the right number of times on the subclasses of
>> Inflater/Deflater.
>
> Overall they look OK.  In your tests, you are testing the number of
> calls for the sub-classes not for Deflate/Inflate so I would either
> update your comments to clarify that or pull them into their own test
> methods
>
I did not understand this. Did you mean I should update the @summary
part of these tests or was it the javadoc on these test methods?

The latest webrev with the above noted changes is available at
http://cr.openjdk.java.net/~jpai/webrev/8225763/3/webrev/

-Jaikiran

[14]RFR of JDK-8227289:Enable assertions for some shell to java conversion tests after JDK-8218960

[Ying Zhou]

Hello,

Please review this patch for updating below tests by adding -ea/esa to 
launcher parameters.


- /test/jdk/java/util/Calendar/SupplementalJapaneseEraTestRun.java
- /test/jdk/java/util/TimeZone/TimeZoneDatePermissionCheckRun.java
- /test/jdk/java/util/TimeZone/Bug8066652Run.java
- 
/test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTestRun.java

- /test/jdk/java/util/ResourceBundle/modules/ModuleTestUtil.java
- /test/jdk/java/util/ResourceBundle/modules/layer/LayerTest.java
- /test/jdk/java/util/ResourceBundle/modules/unnamed/UnNamedTest.java
- /test/jdk/java/util/ResourceBundle/modules/visibility/VisibilityTest.java

JDK Bug: JDK-8227289 

webrev: http://cr.openjdk.java.net/~yzhou/8227289/webrev.00/

Thanks,

Daisy