8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) #130

jaikiran · 2020-09-11T14:17:45Z

Can I please get a review and a sponsor for this patch which fixes the issue reported in https://bugs.openjdk.java.net/browse/JDK-8244706?

The commit here sets the OS header flag to 255 (which represents unknown) as noted in [1]. A new test has been included in this commit to verify the change. Furthermore, this doesn't impact the java.util.zip.GZIPInputStream since it ignores [2] this header value while reading the headers from the stream.

[1] https://tools.ietf.org/html/rfc1952#page-7
[2] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/zip/GZIPInputStream.java#L173

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance)

Reviewers

Lance Andersen (@LanceAndersen - Reviewer)
Brent Christian (@bchristi-git - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/130/head:pull/130
$ git checkout pull/130

bridgekeeper · 2020-09-11T14:19:22Z

👋 Welcome back jpai! 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 · 2020-09-11T14:20:23Z

@jaikiran The following label will be automatically applied to this pull request: core-libs.

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

mlbridge · 2020-09-11T14:23:46Z

Webrevs

03: Full - Incremental (a510dc7)
02: Full - Incremental (7679b11)
01: Full - Incremental (159684d)
00: Full (afad1261b0df657222a4187a6a7484062271c908)

LanceAndersen

Hi Jaikiran,

The change seems fine an inline with the RFC. I can sponsor this once we have another review.

I have run the JCK tests for Zip/Gzip/Jar and Mach5 JDK tier1, tier2 and tier3

openjdk · 2020-09-12T17:39:41Z

@jaikiran This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance)

Reviewed-by: lancea, bchristi

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 53 commits pushed to the master branch:

1086713: 8253206: Enforce whitespace checking for additional source files
e2196f8: 8253162: Make frame::oops_do const
300b851: 8253226: Shenandoah: remove unimplemented ShenandoahStrDedupQueue::verify
c781594: 8253224: Shenandoah: ShenandoahStrDedupQueue destructor calls virtual num_queues()
33f8e70: 8253173: Print heap before and after GC lacks a newline
f509eb0: 8253219: Epsilon: clean up unnecessary includes
7f9b5d9: 8253220: Epsilon: clean up unused code/declarations
e0cf023: 8245309: Re-examine use of ThreadLocalCoders in sun.net.www.ParseUtil
60c4902: 8253016: Box.Filler components should be unfocusable by default
dd43533: 8253222: Shenandoah: unused AlwaysTrueClosure after JDK-8246591
... and 43 more: https://git.openjdk.java.net/jdk/compare/976acddeb5a8df1e868269787c023306aad3fe4a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 10867134fd8eb065e5c78c78e554f085ec3d1bc2.

As you do not have Committer status in this projectan existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@LanceAndersen, @bchristi-git) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

jaikiran · 2020-09-13T02:22:54Z

Thank you Lance for the review and running the tests. I'll wait for another review before initiating the integrate command.

mlbridge · 2020-09-14T22:50:26Z

Mailing list message from Joe Darcy on core-libs-dev:

Should issue have a CSR review for the behavior change?

-Joe

On 9/12/2020 7:25 PM, Jaikiran Pai wrote:

mlbridge · 2020-09-14T22:57:07Z

Mailing list message from Lance Andersen on core-libs-dev:

Hi Joe,

I guess it could. Given it is not used within the implementation(or defined outside of the spec), I will defer to you preference :-)

On Sep 14, 2020, at 6:49 PM, Joe Darcy <joe.darcy at oracle.com> wrote:

Should issue have a CSR review for the behavior change?

-Joe

On 9/12/2020 7:25 PM, Jaikiran Pai wrote:

Best
Lance
------------------

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

mlbridge · 2020-09-14T23:08:30Z

Mailing list message from Joe Darcy on core-libs-dev:

Hi Lance,

I'd prefer to err on the side of having a CSR; thanks,

-Joe

On 9/14/2020 3:55 PM, Lance Andersen wrote:

Hi Joe,

I guess it could. ?Given it is not used within the implementation(or
defined outside of the spec), I will defer to you preference ?:-)

On Sep 14, 2020, at 6:49 PM, Joe Darcy <joe.darcy at oracle.com
<mailto:joe.darcy at oracle.com>> wrote:

Should issue have a CSR review for the behavior change?

-Joe

On 9/12/2020 7:25 PM, Jaikiran Pai wrote:

Best
Lance
------------------

Lance Andersen| Principal?Member of Technical Staff |?+1.781.442.2037
Oracle?Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>

mlbridge · 2020-09-14T23:09:52Z

Mailing list message from Lance Andersen on core-libs-dev:

Hi Joe,

Ok, will create one tomorrow.

Best
Lance

On Sep 14, 2020, at 7:07 PM, Joe Darcy <joe.darcy at oracle.com> wrote:

Hi Lance,

I'd prefer to err on the side of having a CSR; thanks,

-Joe

On 9/14/2020 3:55 PM, Lance Andersen wrote:

Hi Joe,

I guess it could. Given it is not used within the implementation(or defined outside of the spec), I will defer to you preference :-)

On Sep 14, 2020, at 6:49 PM, Joe Darcy <joe.darcy at oracle.com <mailto:joe.darcy at oracle.com>> wrote:

Should issue have a CSR review for the behavior change?

-Joe

On 9/12/2020 7:25 PM, Jaikiran Pai wrote:

Best
Lance
------------------

<oracle_sig_logo.gif>

Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>

Best
Lance
------------------

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

…52 non-compliance) Reviewed-By:

jaikiran · 2020-09-15T12:35:28Z

I received some useful suggestions, offline, from Lance and Brent on this change and have updated this PR to include those suggestions. Please ignore the intermediate commits though (I accidentally added an unwanted file to this PR, which I have then removed in a subsequent commit). This is ready for a review now.

bchristi-git

Reviewed. Thanks for reworking the change into the existing out.write() call.

LanceAndersen

Thank you for making the changes that Brent and I suggested. This looks good. I have submitted, a CSR to track the change.

I have also created a Release Note as well.

Once the CSR has been approved, I will sponsor your change

jaikiran · 2020-09-16T11:38:01Z

Thank you Lance for taking care of the CSR and the release notes. Given that the CSR has been approved, I'm initating the integrate command.

/integrate

openjdk · 2020-09-16T11:39:08Z

@jaikiran
Your change (at version a510dc7) is now ready to be sponsored by a Committer.

LanceAndersen · 2020-09-16T15:08:42Z

/sponsor

openjdk · 2020-09-16T15:11:12Z

@LanceAndersen @jaikiran Since your change was applied there have been 53 commits pushed to the master branch:

1086713: 8253206: Enforce whitespace checking for additional source files
e2196f8: 8253162: Make frame::oops_do const
300b851: 8253226: Shenandoah: remove unimplemented ShenandoahStrDedupQueue::verify
c781594: 8253224: Shenandoah: ShenandoahStrDedupQueue destructor calls virtual num_queues()
33f8e70: 8253173: Print heap before and after GC lacks a newline
f509eb0: 8253219: Epsilon: clean up unnecessary includes
7f9b5d9: 8253220: Epsilon: clean up unused code/declarations
e0cf023: 8245309: Re-examine use of ThreadLocalCoders in sun.net.www.ParseUtil
60c4902: 8253016: Box.Filler components should be unfocusable by default
dd43533: 8253222: Shenandoah: unused AlwaysTrueClosure after JDK-8246591
... and 43 more: https://git.openjdk.java.net/jdk/compare/976acddeb5a8df1e868269787c023306aad3fe4a...master

Your commit was automatically rebased without conflicts.

Pushed as commit e5866aa.

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

…larifications around DistributedSubjectCombiner purpose. openjdk#130

openjdk bot added the rfr Pull request is ready for review label Sep 11, 2020

openjdk bot added the core-libs core-libs-dev@openjdk.org label Sep 11, 2020

LanceAndersen approved these changes Sep 12, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 12, 2020

8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 19…

159684d

…52 non-compliance) Reviewed-By:

jaikiran force-pushed the 8244706 branch from afad126 to 159684d Compare September 15, 2020 12:20

openjdk bot added build build-dev@openjdk.org compiler compiler-dev@openjdk.org labels Sep 15, 2020

jaikiran added 2 commits September 15, 2020 17:57

Improve the comment for the OS_UNKNOWN field

7679b11

Remove unintended file commit

a510dc7

openjdk bot removed build build-dev@openjdk.org compiler compiler-dev@openjdk.org labels Sep 15, 2020

bchristi-git reviewed Sep 15, 2020

View reviewed changes

bchristi-git approved these changes Sep 15, 2020

View reviewed changes

LanceAndersen approved these changes Sep 15, 2020

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 16, 2020

openjdk bot closed this Sep 16, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 16, 2020

jaikiran deleted the 8244706 branch September 16, 2020 15:59

pfirmstone pushed a commit to pfirmstone/jdk-with-authorization that referenced this pull request Nov 18, 2024

Module :: JGDMS Jini Platform - net.jini.security.Security Document c…

fef3879

…larifications around DistributedSubjectCombiner purpose. openjdk#130

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) #130

8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) #130

jaikiran commented Sep 11, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 11, 2020

openjdk bot commented Sep 11, 2020

mlbridge bot commented Sep 11, 2020 •

edited

Loading

LanceAndersen left a comment

openjdk bot commented Sep 12, 2020 •

edited

Loading

jaikiran commented Sep 13, 2020

mlbridge bot commented Sep 14, 2020

mlbridge bot commented Sep 14, 2020

mlbridge bot commented Sep 14, 2020

mlbridge bot commented Sep 14, 2020

jaikiran commented Sep 15, 2020

bchristi-git left a comment

LanceAndersen left a comment

jaikiran commented Sep 16, 2020

openjdk bot commented Sep 16, 2020

LanceAndersen commented Sep 16, 2020

openjdk bot commented Sep 16, 2020

8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) #130

8244706: GZIP "OS" header flag hard-coded to 0 instead of 255 (RFC 1952 non-compliance) #130

Conversation

jaikiran commented Sep 11, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 11, 2020

openjdk bot commented Sep 11, 2020

mlbridge bot commented Sep 11, 2020 • edited Loading

Webrevs

LanceAndersen left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 12, 2020 • edited Loading

jaikiran commented Sep 13, 2020

mlbridge bot commented Sep 14, 2020

mlbridge bot commented Sep 14, 2020

mlbridge bot commented Sep 14, 2020

mlbridge bot commented Sep 14, 2020

jaikiran commented Sep 15, 2020

bchristi-git left a comment

Choose a reason for hiding this comment

LanceAndersen left a comment

Choose a reason for hiding this comment

jaikiran commented Sep 16, 2020

openjdk bot commented Sep 16, 2020

LanceAndersen commented Sep 16, 2020

openjdk bot commented Sep 16, 2020

jaikiran commented Sep 11, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 11, 2020 •

edited

Loading

openjdk bot commented Sep 12, 2020 •

edited

Loading