Skip to content
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

8257707: Fix incorrect format string in Http1HeaderParser #1495

Conversation

turbanoff
Copy link
Member

@turbanoff turbanoff commented Nov 28, 2020

Incorrect format string was found by IntelliJ IDEA inspection Java | Probable bugs | Malformed format string


Progress

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

Issue

  • JDK-8257707: Fix incorrect format string in Http1HeaderParser

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 28, 2020

👋 Welcome back turbanoff! 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
Copy link

openjdk bot commented Nov 28, 2020

@turbanoff The following label will be automatically applied to this pull request:

  • net

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 pull request command.

@openjdk openjdk bot added the net net-dev@openjdk.org label Nov 28, 2020
@shipilev
Copy link
Member

shipilev commented Dec 3, 2020

Submitted: https://bugs.openjdk.java.net/browse/JDK-8257707 -- rename this PR to "8257707: Fix incorrect format string in Http1HeaderParser" to get it hooked here. Also, merge from master to get Windows builds fixed and re-tested.

@turbanoff turbanoff changed the title [PATCH] Fix incorrect format string in Http1HeaderParser 8257707: Fix incorrect format string in Http1HeaderParser Dec 3, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 3, 2020
@turbanoff turbanoff force-pushed the fix_incorrect_format_string_in_Http1HeaderParser branch from 68e5ae9 to 49e18af Compare December 3, 2020 18:27
@mlbridge
Copy link

mlbridge bot commented Dec 3, 2020

Webrevs

@dfuch
Copy link
Member

dfuch commented Dec 3, 2020

Hi Andrey,

Thanks for finding this. Could you run the HttpClient tests - and if they pass I'll sponsor the changes.

jtreg  -verbose:summary -a -ea -esa -agentvm -conc:4 -ignore:quiet -timeout:2 -jdk:<path to the jdk you just built> test/jdk/java/net/httpclient

Alternatively you can run tier2 tests - which include the HttpClient changes:

make test-tier2

(but that's going to run many more tests)

best regards,

-- daniel

@turbanoff
Copy link
Member Author

make test-tier2 fails with error on my machine: :(

user@WORK-PC /cygdrive/f/Projects/official_openjdk
$ make test-tier2
Building target 'test-tier2' in configuration 'windows-x86_64-server-release'
Compiling 127 files for java.compiler
Compiling 10 files for java.instrument
Compiling 18 files for java.datatransfer
Compiling 35 files for java.logging
Compiling 330 files for java.management
Compiling 141 files for java.net.http
Compiling 5 files for java.transaction.xa
Compiling 15 files for java.scripting
Compiling 22 files for java.smartcardio
Compiling 204 files for jdk.internal.vm.ci
Compiling 24 files for jdk.management
Compiling 9 files for jdk.unsupported
Compiling 60 files for jdk.internal.jvmstat
Compiling 131 files for jdk.charsets
Compiling 35 files for jdk.crypto.ec
Compiling 11 files for jdk.crypto.mscapi
Compiling 68 files for jdk.dynalink
Compiling 403 files for jdk.compiler
Compiling 3 files for jdk.internal.ed
Compiling 44 files for jdk.httpserver
Compiling 59 files for jdk.incubator.foreign
Compiling 50 files for jdk.incubator.vector
Compiling 51 files for jdk.internal.opt
Compiling 30 files for jdk.jartool
Compiling 109 files for jdk.internal.le
Compiling 1 files for jdk.jdwp.agent
Compiling 201 files for jdk.jfr
Compiling 4 files for jdk.jsobject
Compiling 8 files for jdk.net
Compiling 1814 files for jdk.localedata
Compiling 2 files for jdk.nio.mapmode
Compiling 25 files for jdk.sctp
Compiling 14 files for jdk.zipfs
Compiling 30 files for java.security.sasl
Compiling 15 files for jdk.attach
Compiling 74 files for jdk.crypto.cryptoki
Compiling 14 files for jdk.management.jfr
Compiling 136 files for jdk.jdeps
Compiling 198 files for java.naming
Compiling 40 files for jdk.jcmd
Compiling 256 files for jdk.jdi
Compiling 131 files for java.rmi
Compiling 82 files for jdk.jlink
Compiling 224 files for java.security.jgss
Compiling 16 files for jdk.naming.dns
Compiling 16 files for java.management.rmi
Compiling 11 files for jdk.jstatd
Compiling 8 files for jdk.naming.rmi
Compiling 31 files for jdk.management.agent
Compiling 30 files for jdk.security.auth
Compiling 16 files for jdk.security.jgss
Compiling 1845 files for java.xml
./build/windows-x86_64-server-release/hotspot/variant-server/gensrc/jvmtifiles/bytecodeInterpreterWithChecks.cpp(3): fatal error C1083: Cannot open include file: 'interpreter/bytecodeInterpreter.cpp': No such file or directory
make[3]: *** [lib/CompileJvm.gmk:143: /cygdrive/f/Projects/official_openjdk/build/windows-x86_64-server-release/hotspot/variant-server/libjvm/objs/bytecodeInterpreterWithChecks.obj] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [make/Main.gmk:252: hotspot-server-libs] Error 2
make[2]: *** Waiting for unfinished jobs....

ERROR: Build failed for target 'test-tier2' in configuration 'windows-x86_64-server-release' (exit code 2)
Stopping sjavac server

=== Output from failing command(s) repeated here ===
* For target hotspot_variant-server_libjvm_objs_bytecodeInterpreterWithChecks.obj:
bytecodeInterpreterWithChecks.cpp
./build/windows-x86_64-server-release/hotspot/variant-server/gensrc/jvmtifiles/bytecodeInterpreterWithChecks.cpp(3): fatal error C1083: Cannot open include file: 'interpreter/bytecodeInterpreter.cpp': No such file or directory

* All command lines available in /cygdrive/f/Projects/official_openjdk/build/windows-x86_64-server-release/make-support/failure-logs.
=== End of repeated output ===

No indication of failed target found.
Hint: Try searching the build log for '] Error'.
Hint: See doc/building.html#troubleshooting for assistance.

make[1]: *** [/cygdrive/f/Projects/official_openjdk/make/Init.gmk:315: main] Ошибка 2
make: *** [/cygdrive/f/Projects/official_openjdk/make/Init.gmk:186: test-tier2] Ошибка 2

@shipilev
Copy link
Member

shipilev commented Dec 4, 2020

=== Output from failing command(s) repeated here ===

  • For target hotspot_variant-server_libjvm_objs_bytecodeInterpreterWithChecks.obj:
    bytecodeInterpreterWithChecks.cpp
    ./build/windows-x86_64-server-release/hotspot/variant-server/gensrc/jvmtifiles/bytecodeInterpreterWithChecks.cpp(3): fatal error C1083: Cannot open include file: 'interpreter/bytecodeInterpreter.cpp': No such file or directory

That's the build failure, actually, and bytecodeInterpreterWithChecks.cpp should have been long gone. Run git clean on your repo?

@shipilev
Copy link
Member

shipilev commented Dec 4, 2020

FWIW, this passes with the patch:

$ CONF=linux-x86_64-server-fastdebug make images run-test TEST=java/net/httpclient
==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/jdk/java/net/httpclient                  219   219     0     0   
==============================
TEST SUCCESS

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@openjdk
Copy link

openjdk bot commented Dec 4, 2020

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

8257707: Fix incorrect format string in Http1HeaderParser

Reviewed-by: shade

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

  • ca40267: 8257509: Strengthen requirements to call G1HeapVerifier::verify(VerifyOption)
  • 4a85514: 8257182: JCK test failures in integer / long rotation tests
  • f33808f: 8257631: C2: Assertion failure in ArrayCopyNode::get_count() during late inlining
  • 417e7e6: 8257632: C2: Late inlining attempt on a call with a dead memory crashes
  • 6845fee: 8257625: C2: Harden input checks in vector intrinsics
  • 4390f2c: 8257630: C2: ReplacedNodes doesn't handle non-CFG multi nodes
  • d08c612: 8257708: Remove redundant unmodifiableSet wrapper from already immutable set returned by Collections.singleton
  • 49f9e57: 8257734: Extraneous output in HmacSHA3_512 constructor
  • f83fd4a: 8257438: Avoid adding duplicate values into extendedKeyCodesSet
  • 37c7835: 8257673: Build fails without shenandoahgc after JDK-8257563
  • ... and 8 more: https://git.openjdk.java.net/jdk/compare/55f5542ca2104df91e14693534cc7b3c36e81953...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.

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 (@shipilev) 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).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 4, 2020
@turbanoff
Copy link
Member Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 4, 2020
@openjdk
Copy link

openjdk bot commented Dec 4, 2020

@turbanoff
Your change (at version 49e18af) is now ready to be sponsored by a Committer.

@shipilev
Copy link
Member

shipilev commented Dec 4, 2020

@dfuch, are you still good with this? Please submit your formal review here :)

@dfuch
Copy link
Member

dfuch commented Dec 4, 2020

Thanks for running the tests Andrei. I will sponsor.

@dfuch
Copy link
Member

dfuch commented Dec 4, 2020

/sponsor

@openjdk openjdk bot closed this Dec 4, 2020
@openjdk 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 Dec 4, 2020
@openjdk
Copy link

openjdk bot commented Dec 4, 2020

@dfuch @turbanoff Since your change was applied there have been 23 commits pushed to the master branch:

  • fbdc187: 8257624: C2: PhaseMacroExpand::eliminate_macro_nodes() crashes on out-of-bounds access into macro node array
  • fd6756e: 8257634: C2: Introduce IncrementalInliningForceCleanup diagnostic flag
  • af6b7f9: 8257743: Minimal build on AArch64 failed with --disable-precompiled-headers
  • d6dd440: 8256264: Printed GlyphVector outline with low DPI has bad quality on Windows
  • 2b4a423: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored
  • ca40267: 8257509: Strengthen requirements to call G1HeapVerifier::verify(VerifyOption)
  • 4a85514: 8257182: JCK test failures in integer / long rotation tests
  • f33808f: 8257631: C2: Assertion failure in ArrayCopyNode::get_count() during late inlining
  • 417e7e6: 8257632: C2: Late inlining attempt on a call with a dead memory crashes
  • 6845fee: 8257625: C2: Harden input checks in vector intrinsics
  • ... and 13 more: https://git.openjdk.java.net/jdk/compare/55f5542ca2104df91e14693534cc7b3c36e81953...master

Your commit was automatically rebased without conflicts.

Pushed as commit c6f93ec.

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

@turbanoff
Copy link
Member Author

Few tests failed with make test-tier2:

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR
>> jtreg:test/jdk:tier2                               3677  3661    16     0 <<
>> jtreg:test/langtools:tier2                           12    10     2     0 <<
>> jtreg:test/jaxp:tier2                               449   444     5     0 <<
==============================
TEST FAILURE

make[1]: *** [/cygdrive/f/Projects/official_openjdk/make/Init.gmk:319: main] Ошибка 1
make: *** [/cygdrive/f/Projects/official_openjdk/make/Init.gmk:186: test-tier2] Ошибка 2

List of failed tests:

java/io/BufferedInputStream/LargeCopyWithMark.java                                                  Failed. Unexpected exit from test [exit code: 1]
java/io/CharArrayReader/OverflowInRead.java                                                         Failed. Unexpected exit from test [exit code: 1]
java/io/File/GetXSpace.java                                                                         Failed. Execution failed: `main' threw exception: java.nio.file.InvalidPathException: Illegal char <:> at index 0: :
java/net/MulticastSocket/MulticastAddresses.java                                                    Failed. Execution failed: `main' threw exception: java.lang.Exception: 1 test(s) failed - see log file.
java/net/MulticastSocket/SetLoopbackMode.java                                                       Failed. Execution failed: `main' threw exception: java.net.NoRouteToHostException: No route to host: no further information
java/nio/file/Files/CopyAndMove.java                                                                Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: AtomicMoveNotSupportedException expected
java/security/AccessController/DoPrivAccompliceTest.java                                            Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: 'user' found in stderr
java/text/Format/CompactNumberFormat/TestCompactNumber.java                                         Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 1
java/text/Format/CompactNumberFormat/TestSpecialValues.java                                         Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 2
java/text/Format/NumberFormat/NumberRegression.java                                                 Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: Uncaught exception thrown in test method Test4068693
java/text/Format/NumberFormat/NumberRoundTrip.java                                                  Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: Uncaught exception thrown in test method TestNumberFormatRoundTrip
javax/sql/testng/test/rowset/cachedrowset/CachedRowSetTests.java                                    Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 2
javax/sql/testng/test/rowset/filteredrowset/FilteredRowSetTests.java                                Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 3
javax/sql/testng/test/rowset/joinrowset/JoinRowSetTests.java                                        Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 3
javax/sql/testng/test/rowset/webrowset/WebRowSetTests.java                                          Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 3
tools/jpackage/share/jdk/jpackage/tests/UnicodeArgsTest.java                                        Failed. Execution failed: `main' threw exception: jdk.jpackage.test.Functional$ExceptionBox: java.lang.RuntimeException: 2 FAILED TESTS

jdk/jshell/ToolBasicTest.java                              Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 4
jdk/jshell/UserJdiUserRemoteTest.java                      Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 1

javax/xml/jaxp/functional/javax/xml/xpath/ptests/XPathExpressionTest.java                Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 2
javax/xml/jaxp/functional/javax/xml/xpath/ptests/XPathFunctionResolverTest.java          Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 1
javax/xml/jaxp/functional/javax/xml/xpath/ptests/XPathTest.java                          Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 2
javax/xml/jaxp/functional/test/astro/AstroTest.java                                      Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 5
javax/xml/jaxp/unittest/transform/TransformerTest.java                                   Failed. Execution failed: `main' threw exception: java.lang.Exception: failures: 1

Looks like they are not related to my changes

@turbanoff turbanoff deleted the fix_incorrect_format_string_in_Http1HeaderParser branch December 5, 2020 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated net net-dev@openjdk.org
3 participants