-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JDK-8291974 PrivateCredentialPermission should not use local variable to enable debugging #10206
Conversation
👋 Welcome back mpowers! A progress list of the required criteria for merging this PR into |
Code change looks good to me. Since a However, now that the serialized form is modified (see https://download.java.net/java/early_access/jdk20/docs/api/serialized-form.html#javax.security.auth.PrivateCredentialPermission), I wonder if we need a small CSR. |
What happens when deserialization encounters a missing field like |
Assign it a default value, |
Webrevs
|
I would write a test which serializes the data (before your change) and deserializes it after. There should be some regression tests that already do that. |
In the problem section part of the CSR, add a sentence on what's wrong with the field. In the spec section, please only keep the removal of the In the compatibility reason, add a sentence on why removing the field has no actual effect on serialization and deserialization. |
@seanjmullan There is an existing test which serializes and deserializes a Are you suggesting that I write and integrate a new test that basically verifies that the Java Serialization Spec works as documented? |
I've added my name as a reviewer of the CSR. As for the test, I think it makes sense to serialize the object in an old JDK, hardcode the bytes in a new test and try to deserialize it, and make sure it equals to an object that is freshly constructed. For the opposite direction, one can write a test in the JDK 17 repo that deserializes a byte array "from the future". This sounds fascinating but probably not worth doing. |
Have we tried this to know for sure that it is ignored? I think you should verify this by serializing w/o this "testing" field and de-serialize it with older releases. |
I wrote a test to deserialize an object created and serialized before the fix. I compare the old object with the current object after the fix. What is important is not the comparison, but that deserialization doesn't throw an exception. |
Does both de-serialization pass, i.e. deserialize the new bytes using old release and deserializing the old bytes using new release? |
I tested old bytes on new release. It passes. I didn't test new bytes on an old release because of Max's "from the future" comment. Honestly, I'm not sure what any of this testing proves. Maybe that the Java Object Serialization Specification isn't broken? |
It's still worth manually testing the opposite direction at least once on your own machine, I just meant it's not worth adding this test into our code repository since it must run with a different JDK. I agree the test should pass if the Java Object Serialization Specification isn't broken, but sometimes our tests are able to expose problems in other areas. |
Fair enough. I'll run a test in the opposite direction. |
Supposedly if the newer bytes doesn't work with the older releases, it is a compatibility format change and you should change the serialVersionID value to indicate it. In this case, some may wonder if it's worthwhile to remove this field. |
As mentioned earlier in the comments... In the current case, since the value of the field is false, it will be hard to observe any change in behavior. |
I ran the test in the reverse direction. No difference in behavior was observed. |
Great, thanks for confirming. |
Thanks for the comments, just want to be sure the old version handles the missing field, that's all. |
* Base64 encoding of Serialized PrivateCredentialPermission object | ||
* before bug fix for JDK-8291974. | ||
*/ | ||
static String before = "rO0ABXNyAC9qYXZheC5zZWN1cml0eS5hdXRoLlByaXZhdGVDcmVkZW50aWFsUGVybWlzc2lvbklV3Hd7UH9MAgADWgAHdGVzdGluZ0wAD2NyZWRlbnRpYWxDbGFzc3QAEkxqYXZhL2xhbmcvU3RyaW5nO0wACnByaW5jaXBhbHN0AA9MamF2YS91dGlsL1NldDt4cgAYamF2YS5zZWN1cml0eS5QZXJtaXNzaW9uscbhPyhXUX4CAAFMAARuYW1lcQB+AAF4cHQAGWNyZWQxIHBjMSAicG4xIiBwYzIgInBuMiIAdAAFY3JlZDFw"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line is a little too long. You can break it into multiple lines and use Base64.getMimeDecoder()
to decode it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or use string concatenation, e.g. "xxx" + "yyy".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good candidate for Text Blocks: https://openjdk.org/jeps/378
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String a = """
xxx
yyy
""";
is not the same as
String a =
"xxx" +
"yyy"
;
a
contains an embedded newline; b
does not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcpowers the idea would be to use
String a = """
xxx\
yyy\
""";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either is OK. That's why I proposed getMimeDecoder
that will skip the newline characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go with Text Blocks and Florent's suggestion, but thanks for mentioning getMimeDecoder
. I'm sure I'll find a use for it in the future.
ObjectInputStream ois = new ObjectInputStream(is); | ||
PrivateCredentialPermission pcp2 = | ||
(PrivateCredentialPermission)ois.readObject(); | ||
is.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary as BAIS.close() is a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
InputStream is = new ByteArrayInputStream(decoded); | ||
|
||
// Deserialize input stream and create a new object. | ||
ObjectInputStream ois = new ObjectInputStream(is); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use try-with resources here so the input stream is automatically closed even if there are exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using try-with resources
System.out.println("Serial2 test succeeded"); | ||
} catch (Exception e) { | ||
e.printStackTrace(); | ||
throw new SecurityException("Serial test failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you include e as the cause (2nd argument) of SecurityException
then you don't need to print the stack trace on line 78.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
* @bug 8291974 | ||
* @summary PrivateCredentialPermission should not use local variable to enable debugging | ||
* implementation-dependent class | ||
* @run main/othervm/policy=Serial.policy Serial2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove main/othervm/policy=Serial.policy
(actually you can remove the whole @run
line then).
The policy argument causes the test to run with a SecurityManager enabled, and there isn't any reason that this test needs to do that AFAICT. Also that policy file is for other tests in this directory for accessing the file system or JAAS credentials, which you are not accessing in this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
import java.io.*; | ||
import java.util.*; | ||
|
||
public class Serial2 implements java.io.Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why this needs to implement Serializable
(I see you probably used the other Serial.java
test in this directory as a template. I don't think that needs it either, but you can leave that as is for now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read your sentence as "leave both for now". Sorry. You mean fix in this PR and leave the other alone.
/sponsor |
@seanjmullan The change author (@mcpowers) must issue an |
Looks good. Go ahead an integrate and I will sponsor. |
/integrate |
@mcpowers This pull request has not yet been marked as ready for integration. |
/sponsor |
@seanjmullan The change author (@mcpowers) must issue an |
@mcpowers This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 23 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@seanjmullan) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/integrate |
/sponsor |
Going to push as commit 8480f87.
Your commit was automatically rebased without conflicts. |
@seanjmullan @mcpowers Pushed as commit 8480f87. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Mailing list message from Sean Mullan on security-dev: On 10/31/22 11:02 AM, Mark Powers wrote:
Yes, I meant remove it from this test if it isn't needed. I think you --Sean |
https://bugs.openjdk.org/browse/JDK-8291974
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10206/head:pull/10206
$ git checkout pull/10206
Update a local copy of the PR:
$ git checkout pull/10206
$ git pull https://git.openjdk.org/jdk pull/10206/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10206
View PR using the GUI difftool:
$ git pr show -t 10206
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10206.diff