8215788: Clarify JarInputStream Manifest access #10045

LanceAndersen · 2022-08-26T15:48:55Z

Please review this PR which updates the JarInputStream class description to clarify when the Manifest is accessible via JarInputStream::getManifest and JarInputStream::get[Jar]Entry.

It is worth noting that with this update, we are finally documenting behavior that dates back to when this class was added to JDK 1.2

Best,
Lance

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires a CSR request to be approved

Issues

JDK-8215788: Clarify JarInputStream Manifest access
JDK-8293753: Clarify JarInputStream Manifest access (CSR)

Reviewers

Weijun Wang (@wangweij - Reviewer) ⚠️ Review applies to aff39596
Sean Mullan (@seanjmullan - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10045/head:pull/10045
$ git checkout pull/10045

Update a local copy of the PR:
$ git checkout pull/10045
$ git pull https://git.openjdk.org/jdk pull/10045/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10045

View PR using the GUI difftool:
$ git pr show -t 10045

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10045.diff

bridgekeeper · 2022-08-26T15:50:40Z

👋 Welcome back lancea! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-08-26T15:53:03Z

@LanceAndersen The following labels will be automatically applied to this pull request:

core-libs
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

src/java.base/share/classes/java/util/jar/JarInputStream.java

mlbridge · 2022-09-13T17:24:39Z

Webrevs

wangweij

Mostly good. Some small comments.

BTW, there are still a lot of linkplain. I'm not sure how javadoc are written for core-libs, but my experience on security-libs is that it's only used when the text is — er — plaintext that's neither a class name nor a method name.

wangweij · 2022-09-13T20:39:31Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

- * can be used to store meta-information about the JAR file and its entries.
+ * The {@code JarInputStream} class, which extends {@linkplain ZipInputStream},
+ * is used to read the contents of a JAR file from an input stream.
+ * It provides support for reading an optional {@linkplain JarFile#MANIFEST_NAME Manifest}


Manifest above is the same as the one below. If the one below is in fixed-width, so should be the one above.

Could you please clarify what you are suggesting. I am using the @linkplain to provide a means to see the actual Manifest name. I am happy to tweak, just need (perhaps an example) of what you would like

On lines 36 and 37, there are two "Manifest". The first uses linkplain so it's using normal font style, the 2nd uses code so it's fixed-width. I would like them to be the same. In fact, I would not use linkplain at all. I only use it when the text is not a Java identifier. For example, {@linkplain SecurityManager the Security Manager}. However in this PR, for all the places where you use it, the text is either a class name or a method name. I would just use link.

On lines 36 and 37, there are two "Manifest". The first uses linkplain so it's using normal font style, the 2nd uses code so it's fixed-width. I would like them to be the same. In fact, I would not use linkplain at all. I only use it when the text is not a Java identifier. For example, {@linkplain SecurityManager the Security Manager}. However in this PR, for all the places where you use it, the text is either a class name or a method name. I would just use link.

Ok, thank you switched to use link from linkplain per your suggestion

wangweij · 2022-09-13T20:42:38Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ *         file
+ *      </li>
+ *      <li>
+ *         All signature-related entries must immediately follow the {@code Manifest}


No need to say must. This is is one of the requirements.

addressed in the next update

wangweij · 2022-09-13T20:44:10Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ * <p>
+ * <b>Note:</b>If a {@code JarEntry} is modified after the Jar file is signed,
+ * a {@linkplain SecurityException} will be thrown when an attempt is made to
+ * read the entry.


Not sure if the the an attempt word is precise. The exception is only thrown when the last byte is read.

Are you suggesting change:

"...will be thrown when an attempt is made to read the entry."

to

"...will be thrown when the entry has been read."

Not sure. Maybe just is read is OK.

Not sure. Maybe just is read is OK.

Updated as "...will be thrown when the entry is read."

LanceAndersen

Max,

Thank you for your feedback. See my responses below

LanceAndersen · 2022-09-13T20:58:04Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

- * can be used to store meta-information about the JAR file and its entries.
+ * The {@code JarInputStream} class, which extends {@linkplain ZipInputStream},
+ * is used to read the contents of a JAR file from an input stream.
+ * It provides support for reading an optional {@linkplain JarFile#MANIFEST_NAME Manifest}


Could you please clarify what you are suggesting. I am using the @linkplain to provide a means to see the actual Manifest name. I am happy to tweak, just need (perhaps an example) of what you would like

LanceAndersen · 2022-09-13T20:58:30Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ *         file
+ *      </li>
+ *      <li>
+ *         All signature-related entries must immediately follow the {@code Manifest}


addressed in the next update

LanceAndersen · 2022-09-13T21:00:50Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ * <p>
+ * <b>Note:</b>If a {@code JarEntry} is modified after the Jar file is signed,
+ * a {@linkplain SecurityException} will be thrown when an attempt is made to
+ * read the entry.


Are you suggesting change:

"...will be thrown when an attempt is made to read the entry."

to

"...will be thrown when the entry has been read."

wangweij

Only tiny comments for the last paragraph.

That said, I have some questions on the other parts of this file:

In getNextEntry, the method spec says "If verification has been enabled, any invalid signature detected while positioning the stream for the next entry will result in an exception." What does this mean?
In getManifest, the method spec says "or null if none". Do we need to say "if not found"?

wangweij · 2022-09-14T02:19:23Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ *  for this entry and {@link JarEntry#getCodeSigners()} may be called to obtain
+ *  the signers.
+ * <p>
+ * <b>Note:</b>If a {@code JarEntry} is modified after the Jar file is signed,


Add a space before If. Capitalize Jar.

LanceAndersen · 2022-09-14T10:35:58Z

Only tiny comments for the last paragraph.

Thank you Max, I addressed the above

That said, I have some questions on the other parts of this file:

1. In `getNextEntry`, the method spec says "If verification has been enabled, any invalid signature detected while positioning the stream for the next entry will result in an exception." What does this mean?

I don't know the history of this comment in the spec and on a quick scan of the code, I am not sure I know either. As our signed JAR expert, I will defer to you (and Alan) if we should remove this or leave for another time to address(which I would prefer if we are not removing the verbiage).

2. In `getManifest`, the method spec says "or null if none". Do we need to say "if not found"?

I think we are OK given the changes to the class description.

AlanBateman · 2022-09-14T12:53:12Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ * <p>
+ * The {@link #getManifest} method will return the {@code Manifest} when it is
+ * the first entry in the stream or {@code META-INF/} is the first entry and
+ * the {@code Manifest} is the second entry within the stream. When the


I think you can insert a comma after "when it is the first entry in the stream"? I think that would make it a bit clearer that there are two cases.

Also I'm wondering if the paragraph should be split into two, meaning "When the Manifest ..." can be the start of a new paragraph. The reason is that the text is trying to explain two things, the first is that the manifest must be at the start of the JAR file, the second is that the ordering that methods are invoked will influence how other methods behave.

I think you can insert a comma after "when it is the first entry in the stream"? I think that would make it a bit clearer that there are two cases.

Done

Also I'm wondering if the paragraph should be split into two, meaning "When the Manifest ..." can be the start of a new paragraph. The reason is that the text is trying to explain two things, the first is that the manifest must be at the start of the JAR file, the second is that the ordering that methods are invoked will influence how other methods behave.

Moved to a new paragraph per your suggestion

wangweij · 2022-09-14T15:52:57Z

My understanding is that getNextEntry does not throw a SecurityException since it should not read anything. If the signature does not match then a SecurityException should be thrown when reading a signature-related file. If a normal entry is modified it should be thrown when reading that entry.

This is a quite big change. I suggest we do not touch it this time.

wangweij

I have no more comment.

LanceAndersen · 2022-09-14T16:39:18Z

I have no more comment.

Thank you Max for your time and input

src/java.base/share/classes/java/util/jar/JarInputStream.java

AlanBateman · 2022-09-16T13:32:41Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ * and {@link #getNextJarEntry()} methods will not return the {@code Manifest}.
+ * If {@code META-INF/} is the first entry in the input stream it will be
+ * also not be returned by {@link #getNextEntry()} and
+ * {@link #getNextJarEntry()}.


JAR files will almost always have the manifest as the first or second entry. It's very hazardous to have getNextEntry/getNextJarEntry work differently when the manifest is present but not at the start. This makes it really hard to specify too. I wonder what the impact would be of changing the implementation so that getNextEntry/getNextJarEntry consistently skip the manifest rather than only when it's at the start. I can't think of anything right now that could dependent on the current behavior where it might or might be returned.

I tend to agree but am reluctant to change 20+ year behavior via this PR as we don't know what we don't know.

Okay, in which case what would you think about just saying that the getNextEntry/getNextJarEntry method do not return the Manifest when it's at the start of the stream, and it's unspecified whether they return the Manifest when it located later in the stream. I think this would give us wriggle room to change it in the future.

Okay, in which case what would you think about just saying that the getNextEntry/getNextJarEntry method do not return the Manifest when it's at the start of the stream, and it's unspecified whether they return the Manifest when it located later in the stream. I think this would give us wriggle room to change it in the future.

Is this any better:

* * When the {@code Manifest} is returned by {@code getManifest()}, the {@link #getNextEntry()} * and {@link #getNextJarEntry()} methods will not return the {@code Manifest}. * If {@code META-INF/} is the first entry in the input stream it will be * also not be returned by {@link #getNextEntry()} and {@link #getNextJarEntry()}. * * @apiNote * It is unspecified whether {@link #getNextEntry()} and * {@link #getNextJarEntry()} will return the {@code Manifest} * when the {@code Manifest} occurs later in the input stream. * * {@link JarEntry#getAttributes()} will return the {@code Manifest}'s * attributes for the current JAR file entry, if any, providing * {@code getManifest()} returns a {@code Manifest} for the JAR file. *

It's a bit better but I think we can make it clearer and also link the JAR Manifest section of the JAR file spec. Can you try this:

* The {@link #getManifest() getManifest} method is used to read the * <a href="{@docRoot}/../specs/jar/jar.html#jar-manifest">JAR Manifest</a> * from the entry {@code META-INF/MANIFEST.MF} when it is the first entry * in the stream (or the second entry in the case that the fist entry is * {@code META-INF/} and the second entry {@code META-INF/MANIFEST.MF}). * * The {@link #getNextJarEntry()} and {@link #getNextEntry()} methods are * used to read JAR file entries from the stream. These methods skip over the * manifest ({@code META-INF/MANIFEST.MF}) when it is at the beginning of the * stream. In other words, these methods do not return an entry for the manifest * when the manifest is the first entry in the stream. If the first entry is * {@code META-INF/} and the second entry is the manifest then both are skipped * over by these methods. Whether these methods skip over the manifest when it * appears later in the stream is not specified.

I think we also have to update getManifest method to align with the above as doesn't say anything about the manifest needing to be at the beginning of the stream.

It's a bit better but I think we can make it clearer and also link the JAR Manifest section of the JAR file spec. Can you try this:

* The {@link #getManifest() getManifest} method is used to read the * <a href="{@docRoot}/../specs/jar/jar.html#jar-manifest">JAR Manifest</a> * from the entry {@code META-INF/MANIFEST.MF} when it is the first entry * in the stream (or the second entry in the case that the fist entry is * {@code META-INF/} and the second entry {@code META-INF/MANIFEST.MF}). * * The {@link #getNextJarEntry()} and {@link #getNextEntry()} methods are * used to read JAR file entries from the stream. These methods skip over the * manifest ({@code META-INF/MANIFEST.MF}) when it is at the beginning of the * stream. In other words, these methods do not return an entry for the manifest * when the manifest is the first entry in the stream. If the first entry is * {@code META-INF/} and the second entry is the manifest then both are skipped * over by these methods. Whether these methods skip over the manifest when it * appears later in the stream is not specified.

Revised using the above

I think we also have to update getManifest method to align with the above as doesn't say anything about the manifest needing to be at the beginning of the stream.

Ok, I added some verbiage similar to what I had originally before we decided to update the class description. Please let me know if this is what you had in mind

Thank you again for your feedback

AlanBateman · 2022-09-18T19:49:44Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ * <p>
+ * {@link JarEntry#getAttributes()} will return the {@code Manifest}'s
+ *  attributes for the current JAR file entry, if any, providing
+ *  {@code getManifest()} returns a {@code Manifest} for the JAR file.


Per-entry attributes is an advanced feature to attempt to bring into the class description. I think it would be simpler to just drop this paragraph. If you really want something on this topic then it would require first describing main vs. per entry attributes and then explaining that the per-entry attributes are obtained with JarEntry::getAttributes when the manifest is at the beginning of the stream.

I can remove, but I am not sure I agree we need to describe main vs attribute here given we are pointing to the Jar spec and if there is any discussion of Pre-entry attributes, it should be in JarEntry IMHO. I guess the clarification I was trying to make, apparently unsuccessfully is that JarEntry will not have access to the attributes if getManifest does not return the Manifest.

I can remove, but I am not sure I agree we need to describe main vs attribute here given we are pointing to the Jar spec and if there is any discussion of Pre-entry attributes, it should be in JarEntry IMHO. I guess the clarification I was trying to make, apparently unsuccessfully is that JarEntry will not have access to the attributes if getManifest does not return the Manifest.

Wording it is hard. The draft wording made it look that must call getManifest, ignore the return value, and then subsequent calls to JarEntry::getAttributes will return attributes for the JAR file entry. I think to properly describe would require more setup to explain that a manifest can optionally include per entry attributes and these are read with JarEntry::getAttributes when the manifest is found at the beginning of the stream..

I can remove, but I am not sure I agree we need to describe main vs attribute here given we are pointing to the Jar spec and if there is any discussion of Pre-entry attributes, it should be in JarEntry IMHO. I guess the clarification I was trying to make, apparently unsuccessfully is that JarEntry will not have access to the attributes if getManifest does not return the Manifest.

Wording it is hard. The draft wording made it look that must call getManifest, ignore the return value, and then subsequent calls to JarEntry::getAttributes will return attributes for the JAR file entry. I think to properly describe would require more setup to explain that a manifest can optionally include per entry attributes and these are read with JarEntry::getAttributes when the manifest is found at the beginning of the stream..

Thinking about this some more, would the following be any better:

* * The {@code Manifest} for a JAR file may include * <a href="{@docRoot}/../specs/jar/jar.html#main-attributes">main</a> and * <a href="{@docRoot}/../specs/jar/jar.html#per-entry-attributes">per entry</a> * attributes. {@link JarEntry#getAttributes()} will return the per entry * attributes for the current JAR file entry, if any, providing * {@code getManifest()} returns the {@code Manifest} for the JAR file. * 

Does this mean that the "Verifying a JarInputStream" should also avoid mentioning "getManifest method returns the manifest"? I understand precisely it should be "getManifest method is able to return the manifest if you call it".

It almost sounds like we should first define the concepts of "well-formed JAR file" and "well-formed signed JAR" and then specify what these methods behave.

Does this mean that the "Verifying a JarInputStream" should also avoid mentioning "getManifest method returns the manifest"? I understand precisely it should be "getManifest method is able to return the manifest if you call it".

See Alan's comment below. I will be copying the wording regarding the Manifest being the 1st/2nd entry

It almost sounds like we should first define the concepts of "well-formed JAR file" and "well-formed signed JAR" and then specify what these methods behave.

The Manifest location requirement is unique to JarInputStream so really want to try to keep these updates to a minimum if at all possible so that we are not copying parts of the Jar spec into the javadoc.

AlanBateman · 2022-09-18T19:49:51Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

- * can be used to store meta-information about the JAR file and its entries.
+ * The {@code JarInputStream} class, which extends {@link ZipInputStream},
+ * is used to read the contents of a JAR file from an input stream.
+ * It provides support for reading an optional {@link JarFile#MANIFEST_NAME Manifest}


What would you think about linking this to {@docroot}/../specs/jar/jar.html#jar-manifest rather tan JarFile#MANIFEST_NAME?

Sure if that is your preference.

src/java.base/share/classes/java/util/jar/JarInputStream.java

AlanBateman · 2022-09-19T06:45:13Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ * </p>
+ *
+ *  <h2>Verifying a JarInputStream</h2>
+ *  {@link #JarInputStream(InputStream, boolean)} may be used to


I realise you've had a few iterations with Max on this section but I'm concerned that the text is telling the reader that they should use the 2-arg constructor to verify the signatures when a JAR is signed. The default is to verify and the main reason to use the 2-arg constructor is when you want to opt out, not opt-in.

I think the intro to this section will need to start with a sentence to say that JAR files can be signed (link to specs/jar/jar.html#signed-jar-file) and that JarInputStream can read a signed JAR from the input stream. As per the description further up, the manifest must be at the start of the stream.

I realise you've had a few iterations with Max on this section but I'm concerned that the text is telling the reader that they should use the 2-arg constructor to verify the signatures when a JAR is signed. The default is to verify and the main reason to use the 2-arg constructor is when you want to opt out, not opt-in.

I think the intro to this section will need to start with a sentence to say that JAR files can be signed (link to specs/jar/jar.html#signed-jar-file) and that JarInputStream can read a signed JAR from the input stream. As per the description further up, the manifest must be at the start of the stream.

OK, will make another pass at this today

OK, will make another pass at this today

I looked at the latest draft (2bafc00). I think it would help if the section "Verifying a JarInputStream" were renamed to "Signed JAR files". The link to getManifest makes the reader wonder if they have to call this method whereas I think what you want to say that the manifest must be at the start of the stream (as per the first section) and then followed by signature entries.

OK, will make another pass at this today

I looked at the latest draft (2bafc00). I think it would help if the section "Verifying a JarInputStream" were renamed to "Signed JAR files".

OK, I will change as you suggest

The link to getManifest makes the reader wonder if they have to call this method whereas I think what you want to say that the manifest must be at the start of the stream (as per the first section) and then followed by signature entries.

The reason I used the getManifest wording is I felt it was easier and less redundant than copying the wording about the Manifest needing to be either the first or second entry (assuming META-INF/ is the first in the stream). However if you prefer that, I will make that change.

Thanks for the updates in 6913871. Just one comment on the updated text, and specifically this sentence:

A {@code JarInputStream} may be used to verify the signatures of a

signed JAR file

assuming the following requirements are met:

The signatures are verified by default so I think you can reduce this down to:

A {@code JarInputStream} verifies the signatures of entries in a Signed JAR file when:

We could say that a program could opt-out of verification by using the 2-arg constructor but that is probably too much to try to fit in here.

Thanks for the updates in 6913871. Just one comment on the updated text, and specifically this sentence:

A {@code JarInputStream} may be used to verify the signatures of a

signed JAR file

assuming the following requirements are met:

The signatures are verified by default so I think you can reduce this down to:

A {@code JarInputStream} verifies the signatures of entries in a Signed JAR file when:

We could say that a program could opt-out of verification by using the 2-arg constructor but that is probably too much to try to fit in here.

Updated per your suggestion above.

src/java.base/share/classes/java/util/jar/JarInputStream.java

LanceAndersen

Thank you Sean for the input. Your suggestions should have all been addressed (hopefully)

src/java.base/share/classes/java/util/jar/JarInputStream.java

LanceAndersen · 2022-09-20T18:08:21Z

Alan,

Assuming we are set with the other changes, did you want to add the following paragraph (or similar) at line 58 to make it clear that if the Manifest is not found, then the JarEntry attributes will not be available:

 * <p>
 * The {@code Manifest} for a JAR file may include
 * <a href="{@docRoot}/../specs/jar/jar.html#main-attributes">main</a> and
 * <a href="{@docRoot}/../specs/jar/jar.html#per-entry-attributes">per entry</a>
 *  attributes. {@link JarEntry#getAttributes()} will return the per entry
 *  attributes for the current JAR file entry, if any, providing
 *  the {@code Manifest} is the first or second entry in the stream as described
 *  above.
 *  </p>
 *

If you have alternate wording, that is fine or we can leave it out entirely.

Hopefully this is the last piece to the update to resolve

Thank you (all) for your time and input

AlanBateman · 2022-09-21T09:23:31Z

Assuming we are set with the other changes, did you want to add the following paragraph (or similar) at line 58 to make it clear that if the Manifest is not found, then the JarEntry attributes will not be available:

This is something the reader can read about in the Signed JAR section of the JAR file spec and I think it's too much to try to include in this section of JarInputStream. If there is a place for this in JarInputStream then it's probably in the method description of getNextJarEntry as that can talk about the properties of the JarEntry that it returns. You could separate that out to a separate issue if you want.

LanceAndersen · 2022-09-21T11:38:56Z

Assuming we are set with the other changes, did you want to add the following paragraph (or similar) at line 58 to make it clear that if the Manifest is not found, then the JarEntry attributes will not be available:

This is something the reader can read about in the Signed JAR section of the JAR file spec and I think it's too much to try to include in this section of JarInputStream. If there is a place for this in JarInputStream then it's probably in the method description of getNextJarEntry as that can talk about the properties of the JarEntry that it returns. You could separate that out to a separate issue if you want.

OK, we can deal with this separately. I guess I was trying to articulate that if the Manifest is not found in beginning of the input stream, then you have no access to the attributes given this is unique to JarInputStream

seanjmullan · 2022-09-23T17:38:47Z

As a side comment, I notice that JarInputStream capitalizes "JAR", whereas JarFile does not ("jar"). We should really be consistent in our javadocs. I think "JAR" is more correct, mainly because that is what the Jar File specification uses.

I am not suggesting you need to change JarFile though this as part of this PR. Mostly noting it for future reference.

LanceAndersen · 2022-09-23T17:52:51Z

As a side comment, I notice that JarInputStream capitalizes "JAR", whereas JarFile does not ("jar"). We should really be consistent in our javadocs. I think "JAR" is more correct, mainly because that is what the Jar File specification uses.

I am not suggesting you need to change JarFile though this as part of this PR. Mostly noting it for future reference.

I think you raise a fair point for a different set up updates. there are places in the JAR File spec which also uses "jar file" vs "JAR file". Similar conversation for "ZIP" vs "Zip" vs "zip".

Too yes, lets agree on the preferred convention and update in one pass through java.util.jar.* and java.util.zip.* and the JAR file spec

seanjmullan

Some minor comments. Consider it reviewed either way.

seanjmullan · 2022-09-23T17:42:04Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ *
+ * <h2>Accessing the Manifest</h2>
+ * <p>
+ * The {@link #getManifest() getManifest} method is used to return the


I think this can be more simply said as "... method returns the ..."

seanjmullan · 2022-09-23T17:45:26Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ * <a href="{@docRoot}/../specs/jar/jar.html#jar-manifest">Manifest</a>
+ * from the entry {@code META-INF/MANIFEST.MF} when it is the first entry
+ * in the stream (or the second entry if the first entry in the stream is
+ * {@code META-INF/} and the second entry is {@code META-INF/MANIFEST.MF}).


Do you also want to say "Otherwise, the method returns null."

Do you also want to say "Otherwise, the method returns null."

The method description has that already, it might be too much detail to include in the class description,.

seanjmullan · 2022-09-23T17:46:34Z

src/java.base/share/classes/java/util/jar/JarInputStream.java

+ * <p> The {@link #getNextJarEntry()} and {@link #getNextEntry()} methods are
+ * used to read JAR file entries from the stream. These methods skip over the


Consider removing "are used to". Just say "... methods read ...".

openjdk · 2022-09-28T19:20:06Z

@LanceAndersen 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:

8215788: Clarify JarInputStream Manifest access

Reviewed-by: weijun, mullan, alanb

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 263 new commits pushed to the master branch:

9309786: 8294472: Remove redundant rawtypes suppression in AbstractChronology
3b7fc80: 8294411: SA should provide more useful info when it fails to start up due to "failed to workaround classshareing"
4fb424b: 8293961: Unused ClassPathZipEntry::contents_do
7515b30: 8279283: BufferedInputStream should override transferTo
7401fe0: 8292912: Make guard card in CardTable inaccessible
70d8428: 8294520: Problemlist java/nio/file/Files/CopyProcFile.java
30e3bf9: 8291805: IGV: Improve Zooming
37f83b9: 8294375: test/jdk/java/nio/channels/vthread/BlockingChannelOps.java is slow
60616f2: 8294059: Serial: Refactor GenCollectedHeap::collect
ea61671: 8294359: Interpreter(AArch64) intrinsify Thread.currentThread()
... and 253 more: https://git.openjdk.org/jdk/compare/da596182a494a36d37030f18328e52e525fc3565...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

LanceAndersen · 2022-09-28T19:21:14Z

/integrate

openjdk · 2022-09-28T19:22:33Z

Going to push as commit 9db95ed.
Since your change was applied there have been 263 commits pushed to the master branch:

9309786: 8294472: Remove redundant rawtypes suppression in AbstractChronology
3b7fc80: 8294411: SA should provide more useful info when it fails to start up due to "failed to workaround classshareing"
4fb424b: 8293961: Unused ClassPathZipEntry::contents_do
7515b30: 8279283: BufferedInputStream should override transferTo
7401fe0: 8292912: Make guard card in CardTable inaccessible
70d8428: 8294520: Problemlist java/nio/file/Files/CopyProcFile.java
30e3bf9: 8291805: IGV: Improve Zooming
37f83b9: 8294375: test/jdk/java/nio/channels/vthread/BlockingChannelOps.java is slow
60616f2: 8294059: Serial: Refactor GenCollectedHeap::collect
ea61671: 8294359: Interpreter(AArch64) intrinsify Thread.currentThread()
... and 253 more: https://git.openjdk.org/jdk/compare/da596182a494a36d37030f18328e52e525fc3565...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-28T19:22:51Z

@LanceAndersen Pushed as commit 9db95ed.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Inital draft to clarify Manifest access

9509130

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Aug 26, 2022

wangweij reviewed Aug 30, 2022

View reviewed changes

src/java.base/share/classes/java/util/jar/JarInputStream.java Outdated Show resolved Hide resolved

src/java.base/share/classes/java/util/jar/JarInputStream.java Outdated Show resolved Hide resolved

LanceAndersen added 3 commits August 31, 2022 14:33

Minor update to the wording for signature entries

ba94fa9

Third draft of the class description

3422e65

Merge branch 'master' into JDK-8215788

38b866f

wangweij reviewed Sep 8, 2022

View reviewed changes

src/java.base/share/classes/java/util/jar/JarInputStream.java Show resolved Hide resolved

src/java.base/share/classes/java/util/jar/JarInputStream.java Outdated Show resolved Hide resolved

LanceAndersen added 2 commits September 12, 2022 15:01

Addressed 3rd draft comments

2756ade

switch linkePlain to code tag

36a74ef

LanceAndersen marked this pull request as ready for review September 13, 2022 17:20

openjdk bot added the rfr Pull request is ready for review label Sep 13, 2022

openjdk bot added the csr Pull request needs approved CSR before integration label Sep 13, 2022

wangweij reviewed Sep 13, 2022

View reviewed changes

LanceAndersen commented Sep 13, 2022

View reviewed changes

changed linkplain to link and updated note wording

cfdfa06

wangweij reviewed Sep 14, 2022

View reviewed changes

Address a couple of typos in Note

aff3959

AlanBateman reviewed Sep 14, 2022

View reviewed changes

wangweij approved these changes Sep 14, 2022

View reviewed changes

Address latest input regarding wording

10ddfd3

AlanBateman reviewed Sep 16, 2022

View reviewed changes

Address additional javadoc comments

6d7795f

openjdk bot removed the rfr Pull request is ready for review label Sep 16, 2022

Incorporated latest round of input

a0b7ae0

openjdk bot added the rfr Pull request is ready for review label Sep 17, 2022

AlanBateman reviewed Sep 18, 2022

View reviewed changes

Updated to address latest feedback

c16e5bb

AlanBateman reviewed Sep 19, 2022

View reviewed changes

Minor clarification for verifying an JarInputStream

bd4e000

seanjmullan reviewed Sep 19, 2022

View reviewed changes

Address Sean's input

2bafc00

LanceAndersen commented Sep 19, 2022

View reviewed changes

LanceAndersen added 2 commits September 20, 2022 12:52

Address comments regarding Verifying a JarInputStreams

6913871

More wordsmithing to the signed jar section

4c4c2a0

seanjmullan reviewed Sep 20, 2022

View reviewed changes

src/java.base/share/classes/java/util/jar/JarInputStream.java Show resolved Hide resolved

Added the verification note from JarFile

18f7e5d

seanjmullan approved these changes Sep 23, 2022

View reviewed changes

AlanBateman approved these changes Sep 28, 2022

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Sep 28, 2022

openjdk bot added the ready Pull request is ready to be integrated label Sep 28, 2022

openjdk bot added the integrated Pull request has been integrated label Sep 28, 2022

openjdk bot closed this Sep 28, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 28, 2022

		* <p> The {@link #getNextJarEntry()} and {@link #getNextEntry()} methods are
		* used to read JAR file entries from the stream. These methods skip over the

8215788: Clarify JarInputStream Manifest access #10045

8215788: Clarify JarInputStream Manifest access #10045

Conversation

LanceAndersen commented Aug 26, 2022 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Aug 26, 2022

openjdk bot commented Aug 26, 2022

mlbridge bot commented Sep 13, 2022 • edited Loading

Webrevs

wangweij left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LanceAndersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LanceAndersen commented Sep 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij commented Sep 14, 2022

wangweij left a comment

Choose a reason for hiding this comment

LanceAndersen commented Sep 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanBateman Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LanceAndersen left a comment

Choose a reason for hiding this comment

LanceAndersen commented Sep 20, 2022

AlanBateman commented Sep 21, 2022

LanceAndersen commented Sep 21, 2022

seanjmullan commented Sep 23, 2022

LanceAndersen commented Sep 23, 2022

seanjmullan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Sep 28, 2022

LanceAndersen commented Sep 28, 2022

openjdk bot commented Sep 28, 2022

openjdk bot commented Sep 28, 2022

LanceAndersen commented Aug 26, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 13, 2022 •

edited

Loading

wangweij left a comment •

edited

Loading

AlanBateman Sep 19, 2022 •

edited

Loading