Re: RFR: 8325621: Improve jspawnhelper version checks [v4]

2024-03-14 Thread Erik Joelsson
On Wed, 13 Mar 2024 17:11:25 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 10 commits:
> 
>  - Merge branch 'master' into JDK-8325621-2
>  - Update style and improve error messages
>  - Print to stdout instead of stderr
>  - Compare version using VERSION_STRING
>  - Code cleanup
>  - Remove string.h include
>  - Update test
>  - Pass version as integer instead of string
>  - Read in version using int instead of string
>  - 8325621: Improve jspawnhelper version checks

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1936699901


Re: RFR: 8325621: Improve jspawnhelper version checks [v2]

2024-03-14 Thread Aleksey Shipilev
On Tue, 12 Mar 2024 18:42:48 GMT, Erik Joelsson  wrote:

>> Chad Rakoczy has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Code cleanup
>
> I'm fine with just using VERSION_FEATURE. I think it's a simple and 
> straightforward enough simplification/approximation. If we think we need a 
> more exact version comparison, then I think we should use one of the version 
> strings instead of arbitrarily picking a set of version variables.

@erikj79, are you good with this version?

-

PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1996757094


Re: RFR: 8325621: Improve jspawnhelper version checks [v4]

2024-03-13 Thread Magnus Ihse Bursie
On Wed, 13 Mar 2024 17:11:25 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 10 commits:
> 
>  - Merge branch 'master' into JDK-8325621-2
>  - Update style and improve error messages
>  - Print to stdout instead of stderr
>  - Compare version using VERSION_STRING
>  - Code cleanup
>  - Remove string.h include
>  - Update test
>  - Pass version as integer instead of string
>  - Read in version using int instead of string
>  - 8325621: Improve jspawnhelper version checks

Build changes look good now.

-

Marked as reviewed by ihse (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1935108939


Re: RFR: 8325621: Improve jspawnhelper version checks [v4]

2024-03-13 Thread Roger Riggs
On Wed, 13 Mar 2024 17:11:25 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 10 commits:
> 
>  - Merge branch 'master' into JDK-8325621-2
>  - Update style and improve error messages
>  - Print to stdout instead of stderr
>  - Compare version using VERSION_STRING
>  - Code cleanup
>  - Remove string.h include
>  - Update test
>  - Pass version as integer instead of string
>  - Read in version using int instead of string
>  - 8325621: Improve jspawnhelper version checks

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1935071268


Re: RFR: 8325621: Improve jspawnhelper version checks [v4]

2024-03-13 Thread Aleksey Shipilev
On Wed, 13 Mar 2024 17:11:25 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 10 commits:
> 
>  - Merge branch 'master' into JDK-8325621-2
>  - Update style and improve error messages
>  - Print to stdout instead of stderr
>  - Compare version using VERSION_STRING
>  - Code cleanup
>  - Remove string.h include
>  - Update test
>  - Pass version as integer instead of string
>  - Read in version using int instead of string
>  - 8325621: Improve jspawnhelper version checks

I am good with this version, thanks!

-

Marked as reviewed by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1934831130


Re: RFR: 8325621: Improve jspawnhelper version checks [v4]

2024-03-13 Thread Chad Rakoczy
> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
> 
> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
> the same. Updates test to include check for version. Also tested manually by 
> replacing jspawnhelper with incorrect version to confirm that check works.

Chad Rakoczy has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 10 commits:

 - Merge branch 'master' into JDK-8325621-2
 - Update style and improve error messages
 - Print to stdout instead of stderr
 - Compare version using VERSION_STRING
 - Code cleanup
 - Remove string.h include
 - Update test
 - Pass version as integer instead of string
 - Read in version using int instead of string
 - 8325621: Improve jspawnhelper version checks

-

Changes: https://git.openjdk.org/jdk/pull/18204/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=18204=03
  Stats: 59 lines in 5 files changed: 47 ins; 4 del; 8 mod
  Patch: https://git.openjdk.org/jdk/pull/18204.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18204/head:pull/18204

PR: https://git.openjdk.org/jdk/pull/18204


Re: RFR: 8325621: Improve jspawnhelper version checks [v3]

2024-03-13 Thread Magnus Ihse Bursie
On Wed, 13 Mar 2024 09:49:13 GMT, Aleksey Shipilev  wrote:

>> make/modules/java.base/Launcher.gmk line 81:
>> 
>>> 79:   SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \
>>> 80:   OPTIMIZATION := LOW, \
>>> 81:   CFLAGS := $(CFLAGS_JDKEXE) \
>> 
>> There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing 
>> like flags we try to fill the line.
>> Also, the indentation rules are that a broken line should be indented with 
>> four spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk.
>
> My fault, I suggested Chad to do this. So what's the preferred formatting 
> here? Like BUILD_JEXEC block above does it?
> 
> 
>   CFLAGS := $(CFLAGS_JDKEXE) $(VERSION_CFLAGS) \
>   -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava, \

Yeah, that looks good. I was thinking just appending it at the end like:


 CFLAGS := $(CFLAGS_JDKEXE) -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava \
  $(VERSION_CFLAGS), \


but your version definitely looks better!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522963660


Re: RFR: 8325621: Improve jspawnhelper version checks [v3]

2024-03-13 Thread Aleksey Shipilev
On Tue, 12 Mar 2024 19:22:25 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Print to stdout instead of stderr
>  - Compare version using VERSION_STRING

I was thinking what would happen for the first time we upgrade jspawnhelper 
like this. There would be no helpful message then, the `jspawnhelper` would 
just exit on argument count check. So I suggest we polish the failure messages 
a bit: always report version at `shutItDown`, and report more verbose failure 
messages too.

See: 
[jspawnhelper-messages-1.patch](https://github.com/openjdk/jdk/files/14586196/jspawnhelper-messages-1.patch)

Older JDK invoking new jspawnhelper would then fail like:


% build/macosx-aarch64-server-fastdebug/images/jdk/lib/jspawnhelper 1:1:1
Incorrect number of arguments: 2
jspawnhelper version 23-internal-adhoc.shipilev.shipilev-jdk
This command is not for general use and should only be run as the result of a 
call to
ProcessBuilder.start() or Runtime.exec() in a java application

-

PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1994051562


Re: RFR: 8325621: Improve jspawnhelper version checks [v3]

2024-03-13 Thread Aleksey Shipilev
On Tue, 12 Mar 2024 23:44:45 GMT, Magnus Ihse Bursie  wrote:

>> Chad Rakoczy has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Print to stdout instead of stderr
>>  - Compare version using VERSION_STRING
>
> make/modules/java.base/Launcher.gmk line 81:
> 
>> 79:   SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \
>> 80:   OPTIMIZATION := LOW, \
>> 81:   CFLAGS := $(CFLAGS_JDKEXE) \
> 
> There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing 
> like flags we try to fill the line.
> Also, the indentation rules are that a broken line should be indented with 
> four spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk.

My fault, I suggested Chad to do this. So what's the preferred formatting here? 
Like BUILD_JEXEC block above does it?


  CFLAGS := $(CFLAGS_JDKEXE) $(VERSION_CFLAGS) \
  -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava, \

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522886607


Re: RFR: 8325621: Improve jspawnhelper version checks [v3]

2024-03-12 Thread Magnus Ihse Bursie
On Tue, 12 Mar 2024 19:22:25 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Print to stdout instead of stderr
>  - Compare version using VERSION_STRING

make/modules/java.base/Launcher.gmk line 81:

> 79:   SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \
> 80:   OPTIMIZATION := LOW, \
> 81:   CFLAGS := $(CFLAGS_JDKEXE) \

There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing like 
flags we try to fill the line.
Also, the indentation rules are that a broken line should be indented with four 
spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522267500


Re: RFR: 8325621: Improve jspawnhelper version checks [v3]

2024-03-12 Thread Chad Rakoczy
> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
> 
> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
> the same. Updates test to include check for version. Also tested manually by 
> replacing jspawnhelper with incorrect version to confirm that check works.

Chad Rakoczy has updated the pull request incrementally with two additional 
commits since the last revision:

 - Print to stdout instead of stderr
 - Compare version using VERSION_STRING

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18204/files
  - new: https://git.openjdk.org/jdk/pull/18204/files/7bbcc08f..62529391

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18204=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=18204=01-02

  Stats: 71 lines in 5 files changed: 12 ins; 41 del; 18 mod
  Patch: https://git.openjdk.org/jdk/pull/18204.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18204/head:pull/18204

PR: https://git.openjdk.org/jdk/pull/18204


Re: RFR: 8325621: Improve jspawnhelper version checks [v2]

2024-03-12 Thread Erik Joelsson
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Code cleanup

I'm fine with just using VERSION_FEATURE. I think it's a simple and 
straightforward enough simplification/approximation. If we think we need a more 
exact version comparison, then I think we should use one of the version strings 
instead of arbitrarily picking a set of version variables.

-

PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1992318595


Re: RFR: 8325621: Improve jspawnhelper version checks [v2]

2024-03-12 Thread Chad Rakoczy
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Code cleanup

Thanks for all this feedback! I am going to try and see if we can pass the full 
version string.

-

PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1992241562


Re: RFR: 8325621: Improve jspawnhelper version checks [v2]

2024-03-12 Thread Magnus Ihse Bursie
On Mon, 11 Mar 2024 20:04:48 GMT, Roger Riggs  wrote:

>> Chad Rakoczy has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Code cleanup
>
> make/modules/java.base/Launcher.gmk line 85:
> 
>> 83:   -DVERSION_INTERIM=$(VERSION_INTERIM) \
>> 84:   -DVERSION_UPDATE=$(VERSION_UPDATE) \
>> 85:   -DVERSION_PATCH=$(VERSION_PATCH), \
> 
> Using all 4 is way overkill for the problem at hand.  Just the 
> FEATURE_VERSION is sufficient.
> We all know better than to make incompatible changes in minor versions let 
> alone update or patch version.

There is already a `$(VERSION_CFLAGS)` variable defined. It will set all those 
(and some more). Please use it instead. But then, as Roger says, it is probably 
overkill to *check* anything but the feature version.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1521525658


Re: RFR: 8325621: Improve jspawnhelper version checks [v2]

2024-03-11 Thread Roger Riggs
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Code cleanup

make/modules/java.base/Launcher.gmk line 85:

> 83:   -DVERSION_INTERIM=$(VERSION_INTERIM) \
> 84:   -DVERSION_UPDATE=$(VERSION_UPDATE) \
> 85:   -DVERSION_PATCH=$(VERSION_PATCH), \

Using all 4 is way overkill for the problem at hand.  Just the FEATURE_VERSION 
is sufficient.
We all know better than to make incompatible changes in minor versions let 
alone update or patch version.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1520364358


Re: RFR: 8325621: Improve jspawnhelper version checks [v2]

2024-03-11 Thread Erik Joelsson
On Mon, 11 Mar 2024 18:36:57 GMT, Aleksey Shipilev  wrote:

> > If you really want to require an exact match for jspawnhelper, then these 4 
> > numbers aren't necessarily enough, but a rather arbitrarily chosen 
> > approximation.
> 
> I think for our purposes, a version number quadruplet is enough to provide 
> basic level of safety for jspawnhelper protocol updates across JDK versions, 
> without version checking code being a safety problem itself.
> 
> Putting in the full version string would raise more questions, at least for 
> me. For example, are we guaranteed that version string always fits the 
> argument line? Probably so. Would we get some interesting (Unicode?) symbols 
> in version string that would break somewhere along the way? Would we need to 
> dynamically allocate the buffer for arguments, instead of allocating enough 
> to hold the version _integers_? Would we make a mistake while doing so? Etc.
> 
> All in all, it feels better to silently accept some version mismatches not 
> captured by the version quadruplet, rather than fail trying to do a perfect 
> match.

Legal characters for the version string are defined in 
https://openjdk.org/jeps/223 and verified by configure. There is no risk of 
weird unicode being included. If the full version string is too long, you could 
consider using `VERSION_SHORT`, which drops the $OPT, and the $BUILD part. 

I don't think we need to dynamically allocate the buffer. The required buffer 
length is known at compile time as the length of the expected version string 
(which we get as a command line macro) +1. If the user supplies more, we don't 
need to compare the rest of the string as we already know it's not equal.

To me, hardcoding of 4 version digits raises more questions. We spend 
unnecessary time and effort specifically serializing and de-serializing 4 
numbers, always maintaining the correct order. To me that's brittle code. If 
the version elements, or how we use them, were to change in the future, then 
this code needs to be carefully revisited, whereas if we just used a standard 
version string, it would continue to work.

-

PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1989320103


Re: RFR: 8325621: Improve jspawnhelper version checks [v2]

2024-03-11 Thread Aleksey Shipilev
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Code cleanup

src/java.base/unix/native/jspawnhelper/jspawnhelper.c line 173:

> 171: if (jdk_feature != VERSION_FEATURE || jdk_interim != 
> VERSION_INTERIM || jdk_update != VERSION_UPDATE || jdk_patch != 
> VERSION_PATCH) {
> 172: fprintf(stderr, "Expected jspawnhelper for Java %d.%d.%d.%d, 
> ", jdk_feature, jdk_interim, jdk_update, jdk_patch);
> 173: fprintf(stderr, "but jspawnhelper for Java %d.%d.%d.%d was 
> found.\n", VERSION_FEATURE, VERSION_INTERIM, VERSION_UPDATE, VERSION_PATCH);

Minor: It is a bit odd to see that jspawnhelper found its own version odd. I'd 
say:


fprintf(stderr, "jspawnhelper version check failed. jspawnhelper version: 
%d.%d.%d.%d, JDK version: %d.%d.%d.%d\n",
VERSION_FEATURE, VERSION_INTERIM, VERSION_UPDATE, VERSION_PATCH,
jdk_feature, jdk_interim, jdk_update, jdk_patch);

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1520296858


Re: RFR: 8325621: Improve jspawnhelper version checks

2024-03-11 Thread Aleksey Shipilev
On Mon, 11 Mar 2024 18:56:21 GMT, Bernd  wrote:

> with a protocol version you don’t have to care about micro versions and also 
> it is more tolerant about the usual cpu updates which do not introduce 
> incompatibilities most of the time.

I think we can only reasonably guarantee that jspawnhelper built for a 
particular JDK is compatible. Protocol version does not exactly capture that: 
there might be a slight change in JDK that jspawnhelper needs to be aware 
about, but which does not readily manifest in handshake protocol. Version 
quadruplet seems to get us what we want automatically. Plus, as you said, 
protocol number would probably communicate a wrong message that there is some 
guarantee about the protocol.

> btw just as a datapoint: we run into this issue with a longrunning Gerrit 
> server which could no longer invoke external ssh client for incoming hooks 
> (ad did not log this). It was not expected to use the system-vm which was 
> updated on the running system by ubuntu.

Ouch!

-

PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1989231670


Re: RFR: 8325621: Improve jspawnhelper version checks [v2]

2024-03-11 Thread Chad Rakoczy
> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
> 
> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
> the same. Updates test to include check for version. Also tested manually by 
> replacing jspawnhelper with incorrect version to confirm that check works.

Chad Rakoczy has updated the pull request incrementally with one additional 
commit since the last revision:

  Code cleanup

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18204/files
  - new: https://git.openjdk.org/jdk/pull/18204/files/cf3c20fb..7bbcc08f

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18204=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=18204=00-01

  Stats: 48 lines in 4 files changed: 16 ins; 24 del; 8 mod
  Patch: https://git.openjdk.org/jdk/pull/18204.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18204/head:pull/18204

PR: https://git.openjdk.org/jdk/pull/18204


Re: RFR: 8325621: Improve jspawnhelper version checks

2024-03-11 Thread Bernd
On Mon, 11 Mar 2024 17:46:14 GMT, Chad Rakoczy  wrote:

> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
> 
> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
> the same. Updates test to include check for version. Also tested manually by 
> replacing jspawnhelper with incorrect version to confirm that check works.

Since incompatible changes here are seldom, another option would be to set/send 
a protocol version. Because if you reject an execute() on each mismatch or if 
only a incompatible execute() fails is both undesireable, but much more often 
with version compare (of course third behavior crash/corruption would be bad, 
but the bugfix should avoid that).

with a protocol version you don’t have to care about micro versions and also it 
is more tolerant about the usual cpu updates which do not introduce 
incompatibilities most of the time.

having said that, if you don’t want to introduce a protocol version and don’t 
want to gurantee this interface - the version quadruple would be fine for the 
most common cases of quarterly security updates.

btw just as a datapoint: we run into this issue with a longrunning Gerrit 
server which could no longer invoke external ssh client for incoming hooks (ad 
did not log this). It was not expected to use the system-vm which was updated 
on the running system by ubuntu.

-

PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1989211732


Re: RFR: 8325621: Improve jspawnhelper version checks

2024-03-11 Thread Erik Joelsson
On Mon, 11 Mar 2024 17:46:14 GMT, Chad Rakoczy  wrote:

> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
> 
> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
> the same. Updates test to include check for version. Also tested manually by 
> replacing jspawnhelper with incorrect version to confirm that check works.

If you really want to require an exact match for jspawnhelper, then these 4 
numbers aren't necessarily enough, but a rather arbitrarily chosen 
approximation. We have 3 more potential dotted numbers (extra1-3) as well as 
the OPT string that could potentially describe a different build. 

I saw someone argued for using separated version variables for comparison in an 
issue comment, but I'm not sure I agree. We have a pretty well defined version 
string so comparing that would be much easier, and basically guaranteed to 
accomplish what I understand to be the goal of this change. Comparing any 
subset of other version variables will always just be an approximation, and 
while an approximation may be fine, it's difficult to predict exactly how that 
will play out in the future.

src/java.base/unix/native/jspawnhelper/jspawnhelper.c line 172:

> 170: // Check that JDK version and jspawnhelper version are the same
> 171: if (jdk_feature != VERSION_FEATURE || jdk_interim != 
> VERSION_INTERIM || jdk_update != VERSION_UPDATE || jdk_patch != 
> VERSION_PATCH) {
> 172: fprintf(stderr, "Expected jspawnhelper for Java %d.%d.%d+%d, 
> ", jdk_feature, jdk_interim, jdk_update, jdk_patch);

This isn't correct. All of feature, interim, update and patch are always dot 
separated. The `+` is the separator for the build number.

-

PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1928706835
PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1520217129


Re: RFR: 8325621: Improve jspawnhelper version checks

2024-03-11 Thread Aleksey Shipilev
On Mon, 11 Mar 2024 18:16:53 GMT, Erik Joelsson  wrote:

> If you really want to require an exact match for jspawnhelper, then these 4 
> numbers aren't necessarily enough, but a rather arbitrarily chosen 
> approximation.

I think for our purposes, a version number quadruplet is enough to provide 
basic level of safety for jspawnhelper protocol updates across JDK versions, 
without version checking code being a safety problem itself. 

Putting in the full version string would raise more questions, at least for me. 
For example, are we guaranteed that version string always fits the argument 
line? Probably so. Would we get some interesting (Unicode?) symbols in version 
string that would break somewhere along the way? Would we need to dynamically 
allocate the buffer for arguments, instead of allocating enough to hold the 
version _integers_? Would we make a mistake while doing so? Etc.

All in all, it feels better to silently accept some version mismatches not 
captured by the version quadruplet, rather than fail trying to do a perfect 
match.

-

PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1989179598