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

jetty.sh doesn't work with JETTY_USER in Jetty 10.0.17 thru Jetty 12.0.2 #10696

Closed
joakime opened this issue Oct 10, 2023 · 16 comments
Closed

jetty.sh doesn't work with JETTY_USER in Jetty 10.0.17 thru Jetty 12.0.2 #10696

joakime opened this issue Oct 10, 2023 · 16 comments
Assignees
Labels
Bug For general bugs on Jetty side High Priority

Comments

@joakime
Copy link
Contributor

joakime commented Oct 10, 2023

Jetty version(s)
10.0.17, 11.0.17, 12.0.2

Jetty Environment
All

Java version/vendor (use: java -version)
All

OS type/version
Linux

Description
The jetty.sh is misbehaving when it comes to use of the JETTY_USER to switch users.

Some issues discovered:

  • The PID file cannot be properly created. (likely due to touch pid behavior creating an empty pid on the root uid)
  • The stop command doesn't work.
  • A module file with unusual jvm command line arguments breaks start-stop-daemon
@joakime joakime added the Bug For general bugs on Jetty side label Oct 10, 2023
@joakime joakime self-assigned this Oct 10, 2023
@joakime joakime moved this to 🏗 In progress in Jetty 10.0.18 / 11.0.18 Oct 10, 2023
@joakime joakime moved this to 🏗 In progress in Jetty 12.0.3 Oct 10, 2023
@sbordet
Copy link
Contributor

sbordet commented Oct 10, 2023

Example module file with options that are not compatible with jetty.sh (last 2 lines contain "weird" characters):

[exec]
-showversion
-Xmx512m
-XX:+PrintCommandLineFlags
-XX:+UseZGC
-XX:+ZGenerational
-Xlog:gc*:file=logs/gc.log:time,level,tags
-XX:ErrorFile=/srv/www/websites/jvm_crash_pid_%p.log

@joakime
Copy link
Contributor Author

joakime commented Oct 10, 2023

The problem comes from the combination of ...

  • The root user kicks off the jetty.sh
  • The start-stop-daemon exists on the OS
  • The JETTY_USER configuration option is specified to switch users.

On Jetty 10.0.17 / 11.0.17 / 12.0.2 the touch $JETTY_PID gets created by root, then the user switches, and then the user process PidFile cannot write to the pid file.
Failing the startup.

I'm wondering why we are even using start-stop-daemon.
It looks like start-stop-daemon wants total control over the pid file.
And it even validates that the pid file is root created / owned before trusting it (eg: stop)

Do we need start-stop-daemon anymore?
I suspect we do, but in a tinier set of cases than in the past.

Looking at the history of start-stop-daemon:

@joakime
Copy link
Contributor Author

joakime commented Oct 10, 2023

I'm moving the touch <file> tests to only execute if the JETTY_USER is unspecified. (ie: no user id change)
I'm also moving the --module=pid out from jetty.conf into the flows that do not use start-stop-daemon
I'm also re-introducing the START_STOP_DAEMON=1 to the jetty.sh to allow disabling its based on configuration.

joakime added a commit that referenced this issue Oct 10, 2023
joakime added a commit that referenced this issue Oct 10, 2023
joakime added a commit that referenced this issue Oct 10, 2023
@joakime
Copy link
Contributor Author

joakime commented Oct 10, 2023

Opened PR #10700

joakime added a commit that referenced this issue Oct 10, 2023
…t, or process will switch to JETTY_USER

Signed-off-by: Joakim Erdfelt <[email protected]>
joakime added a commit that referenced this issue Oct 10, 2023
joakime added a commit that referenced this issue Oct 10, 2023
joakime added a commit that referenced this issue Oct 10, 2023
@gregw
Copy link
Contributor

gregw commented Oct 10, 2023

So I'm walking through this from first principles.
Thus firstly I've done some testing with a test.mod file containing:

[exec]
-showversion
-Xmx512m
-XX:+PrintCommandLineFlags
-XX:+UseZGC
-Xlog:gc*:file=logs/gc.log:time,level,tags
-XX:ErrorFile=/srv/www/websites/jvm_crash_pid_%p.log

The dry run produces the correctly quoted:

/usr/lib/jvm/java-21-openjdk-amd64/bin/java -Djava.io.tmpdir=/tmp -Djetty.home=/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home -Djetty.base=/home/gregw/src/jetty-10.0.x/jetty-home/target/base -showversion -Xmx512m -XX:+PrintCommandLineFlags -XX:+UseZGC '-Xlog:gc*:file=logs/gc.log:time,level,tags' '-XX:ErrorFile=/srv/www/websites/jvm_crash_pid_%p.log' --class-path /home/gregw/src/jetty-10.0.x/jetty-home/target/base/resources:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/logging/slf4j-api-2.0.5.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/logging/jetty-slf4j-impl-10.0.18-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-servlet-api-4.0.6.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-http-10.0.18-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-server-10.0.18-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-xml-10.0.18-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-util-10.0.18-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-io-10.0.18-SNAPSHOT.jar org.eclipse.jetty.xml.XmlConfiguration java.version=21-ea jetty.base=/home/gregw/src/jetty-10.0.x/jetty-home/target/base jetty.base.uri=file:///home/gregw/src/jetty-10.0.x/jetty-home/target/base jetty.home=/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home jetty.home.uri=file:///home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home jetty.webapp.addServerClasses=org.eclipse.jetty.logging.,file:///home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/logging/,org.slf4j. runtime.feature.alpn=true slf4j.version=2.0.5 /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-bytebufferpool.xml /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-threadpool.xml /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty.xml /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-http.xml

I have run this with bash, sh and zsh without any issue. So I do not think there are any problems with quoting as done by start.jar. @lachlan-roberts can you confirm if the docker start scripts are working OK with these releases and the quoting done?

@lachlan-roberts
Copy link
Contributor

@gregw the docker images are working, but they do not use jetty.sh

@gregw
Copy link
Contributor

gregw commented Oct 10, 2023

@gregw the docker images are working, but they do not use jetty.sh

Great. I asked because they do use --dry-run, so it is a good test that quoting is correct there.

@gregw
Copy link
Contributor

gregw commented Oct 10, 2023

I simplified my module a bit for less verbose testing and also used full paths:

[exec]
-showversion
-XX:+PrintCommandLineFlags
-Xlog:gc*:file=/tmp/logs/gc.log:time,level,tags
-XX:ErrorFile=/tmp/logs/jvm_crash_pid_%p.log

with this, I was able to cut/paste the dry-run output to start-stop-daemon and it worked fine:

[1073] start-stop-daemon --start --name test --startas /usr/lib/jvm/java-21-openjdk-amd64/bin/java -- -Djava.io.tmpdir=/tmp -Djetty.home=/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home -Djetty.base=/home/gregw/src/jetty-10.0.x/jetty-home/target/base -showversion -XX:+PrintCommandLineFlags '-Xlog:gc*:file=/tmp/logs/gc.log:time,level,tags' '-XX:ErrorFile=/tmp/logs/jvm_crash_pid_%p.log' --class-path /home/gregw/src/jetty-10.0.x/jetty-home/target/base/resources:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/logging/slf4j-api-2.0.5.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/logging/jetty-slf4j-impl-10.0.18-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-servlet-api-4.0.6.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-http-10.0.18-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-server-10.0.18-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-xml-10.0.18-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-util-10.0.18-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-io-10.0.18-SNAPSHOT.jar org.eclipse.jetty.xml.XmlConfiguration java.version=21-ea jetty.base=/home/gregw/src/jetty-10.0.x/jetty-home/target/base jetty.base.uri=file:///home/gregw/src/jetty-10.0.x/jetty-home/target/base jetty.home=/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home jetty.home.uri=file:///home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home jetty.webapp.addServerClasses=org.eclipse.jetty.logging.,file:///home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/logging/,org.slf4j. runtime.feature.alpn=true slf4j.version=2.0.5 /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-bytebufferpool.xml /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-threadpool.xml /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty.xml /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-http.xml
-XX:ConcGCThreads=4 -XX:ErrorFile=/tmp/logs/jvm_crash_pid_%p.log -XX:G1ConcRefinementThreads=15 -XX:GCDrainStackTargetSize=64 -XX:InitialHeapSize=519190656 -XX:MarkStackSize=4194304 -XX:MaxHeapSize=8307050496 -XX:MinHeapSize=6815736 -XX:+PrintCommandLineFlags -XX:ReservedCodeCacheSize=251658240 -XX:+SegmentedCodeCache -XX:+UseCompressedClassPointers -XX:+UseCompressedOops -XX:+UseG1GC 
openjdk version "21-ea" 2023-09-19
OpenJDK Runtime Environment (build 21-ea+14-Ubuntu-0ubuntu1)
OpenJDK 64-Bit Server VM (build 21-ea+14-Ubuntu-0ubuntu1, mixed mode, sharing)
2023-10-11 10:46:24.006:INFO :oejs.Server:main: jetty-10.0.18-SNAPSHOT; built: 2023-10-10T22:24:00.019Z; git: d3aa35511deae8b1d3d27ddef79bb54a38ea8560; jvm 21-ea+14-Ubuntu-0ubuntu1
2023-10-11 10:46:24.033:INFO :oejs.AbstractConnector:main: Started ServerConnector@55a1c291{HTTP/1.1, (http/1.1)}{0.0.0.0:8080}
2023-10-11 10:46:24.048:INFO :oejs.Server:main: Started Server@1d7acb34{STARTING}[10.0.18-SNAPSHOT,sto=5000] @276ms

But the problem comes when trying to use the dry-run via a shell mechanism. All the following failed:

start-stop-daemon --start --name test --startas $(java -jar ../jetty-home/start.jar --dry-run=java) -- $(java -jar ../jetty-home/start.jar --dry-run=opts,path,main,args)
start-stop-daemon --start --name test --startas $(java -jar ../jetty-home/start.jar --dry-run=java) -- "$(java -jar ../jetty-home/start.jar --dry-run=opts,path,main,args)"
start-stop-daemon --start --name test --startas $(java -jar ../jetty-home/start.jar --dry-run=java) -- '$(java -jar ../jetty-home/start.jar --dry-run=opts,path,main,args)'

However the following does appear to work:

java -jar ../jetty-home/start.jar --dry-run=opts,path,main,args | xargs start-stop-daemon --start --name test --startas $(java -jar ../jetty-home/start.jar --dry-run=java) --

@lachlan-roberts can you comment on usage of xargs in docker? was that finally accepted or is something else done?

@lachlan-roberts
Copy link
Contributor

@gregw I don't think we use xargs anywhere for the docker images.

We use an eval exec to do the --dry-run, the output is put into a $JETTY_START file with exec appended to the start of the file. Then we run this file with . $JETTY_START.

@gregw
Copy link
Contributor

gregw commented Oct 11, 2023

@lachlan-roberts thanks. That technique also works with start-stop-daemon:

[1094] echo -n start-stop-daemon --start --name test --startas $(java -jar ../jetty-home/start.jar --dry-run=java) -- ' ' > /tmp/run

[1095] java -jar ../jetty-home/start.jar --dry-run=opts,path,main,args >> /tmp/run

[1096] . /tmp/run
-XX:ConcGCThreads=4 -XX:ErrorFile=/tmp/logs/jvm_crash_pid_%p.log -XX:G1ConcRefinementThreads=15 -XX:GCDrainStackTargetSize=64 -XX:InitialHeapSize=519190656 -XX:MarkStackSize=4194304 -XX:MaxHeapSize=8307050496 -XX:MinHeapSize=6815736 -XX:+PrintCommandLineFlags -XX:ReservedCodeCacheSize=251658240 -XX:+SegmentedCodeCache -XX:+UseCompressedClassPointers -XX:+UseCompressedOops -XX:+UseG1GC 
openjdk version "21-ea" 2023-09-19
OpenJDK Runtime Environment (build 21-ea+14-Ubuntu-0ubuntu1)
OpenJDK 64-Bit Server VM (build 21-ea+14-Ubuntu-0ubuntu1, mixed mode, sharing)
2023-10-11 12:19:04.173:INFO :oejs.Server:main: jetty-10.0.18-SNAPSHOT; built: 2023-10-10T22:24:00.019Z; git: d3aa35511deae8b1d3d27ddef79bb54a38ea8560; jvm 21-ea+14-Ubuntu-0ubuntu1
2023-10-11 12:19:04.201:INFO :oejs.AbstractConnector:main: Started ServerConnector@55a1c291{HTTP/1.1, (http/1.1)}{0.0.0.0:8080}
2023-10-11 12:19:04.213:INFO :oejs.Server:main: Started Server@1d7acb34{STARTING}[10.0.18-SNAPSHOT,sto=5000] @259ms

@gregw
Copy link
Contributor

gregw commented Oct 11, 2023

@joakime Can you use either the xargs technique or the file technique above to call the start-stop-daemon in a way that better handles quotes and escapes?

@gregw
Copy link
Contributor

gregw commented Oct 11, 2023

@joakime note that you can mix in extra args do something like:

[1101] { echo "-Dfoo=bar" ; java -jar ../jetty-home/start.jar --dry-run=opts,path,main,args ; } | xargs start-stop-daemon --start --name test --startas $(java -jar ../jetty-home/start.jar --dry-run=java) --
-XX:ConcGCThreads=4 -XX:ErrorFile=/tmp/logs/jvm_crash_pid_%p.log -XX:G1ConcRefinementThreads=15 -XX:GCDrainStackTargetSize=64 -XX:InitialHeapSize=519190656 -XX:MarkStackSize=4194304 -XX:MaxHeapSize=8307050496 -XX:MinHeapSize=6815736 -XX:+PrintCommandLineFlags -XX:ReservedCodeCacheSize=251658240 -XX:+SegmentedCodeCache -XX:+UseCompressedClassPointers -XX:+UseCompressedOops -XX:+UseG1GC 
openjdk version "21-ea" 2023-09-19
OpenJDK Runtime Environment (build 21-ea+14-Ubuntu-0ubuntu1)
OpenJDK 64-Bit Server VM (build 21-ea+14-Ubuntu-0ubuntu1, mixed mode, sharing)
2023-10-11 13:25:28.400:INFO :oejs.Server:main: jetty-10.0.18-SNAPSHOT; built: 2023-10-10T22:24:00.019Z; git: d3aa35511deae8b1d3d27ddef79bb54a38ea8560; jvm 21-ea+14-Ubuntu-0ubuntu1
2023-10-11 13:25:28.424:INFO :oejs.AbstractConnector:main: Started ServerConnector@55a1c291{HTTP/1.1, (http/1.1)}{0.0.0.0:8080}
2023-10-11 13:25:28.436:INFO :oejs.Server:main: Started Server@1d7acb34{STARTING}[10.0.18-SNAPSHOT,sto=5000] @236ms

@joakime
Copy link
Contributor Author

joakime commented Oct 11, 2023

Note, that while the initial PR is for jetty-10.0.x, the problem observed with the jvm command line arguments was first noticed on the recent jetty-12.0.2 release while running on JDK 21.

We got a mysterious error along the lines of 'D' is invalid.

Not sure yet if the version of Jetty makes a difference here.

I'll try to replicate (reliably) the fault on Jetty 12.0.2 so we can look deeper into it.

@joakime
Copy link
Contributor Author

joakime commented Oct 11, 2023

I was able to replicate the origin of the "D" issue ...

Oct 11 21:44:36 telesto systemd[1]: Starting LSB: Jetty start script....
Oct 11 21:44:36 telesto website[4114]: Starting Jetty:
Oct 11 21:44:36 telesto website[4163]: start-stop-daemon: invalid option -- 'D'
Oct 11 21:44:36 telesto website[4163]: Try 'start-stop-daemon --help' for more information.
Oct 11 21:44:36 telesto website[4164]: /etc/init.d/website: line 566: --make-pidfile: command not found

Turns out it's a message from start-stop-daemon, it is seeing an argument with a D, which is nonsense to it.
This means the xargs is arriving before the -- argument on the command line, which is bad.

Turns out it's a subtle bug in 12.0.2

https://github.com/eclipse/jetty.project/blob/b01e3611cfcec50c9157c50f6d32a93515e01510/jetty-home/src/main/resources/bin/jetty.sh#L559-L567

The bug is on line 564

@gregw
Copy link
Contributor

gregw commented Oct 12, 2023

@joakime I've found and "fixed" another pathological test case (that we should include). I created a base dir like:

[1205] tree .
.
├── dir one
│   └── test1.xml
├── dir two
│   └── test2.xml
├── etc
│   └── jetty.conf
├── jetty
├── modules
│   └── test.mod
├── resources
│   └── jetty-logging.properties
└── start.d
    ├── server.ini
    └── test.ini

modules/test.mod is

[exec]
-showversion
-XX:+PrintCommandLineFlags
-Xlog:gc*:file=/tmp/logs/gc.log:time,level,tags
-XX:ErrorFile=/tmp/logs/jvm_crash_pid_%p.log

etc/jetty.conf is

# test configurations with spaces
dir one/test1.xml
dir two

The xml files are both just:

<?xml version="1.0"?>
<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "https://www.eclipse.org/jetty/configure_10_0.dtd">

<Configure id="Server" class="org.eclipse.jetty.server.Server">
</Configure>

This died in numerous ways. The "fix" I have is

diff --git a/jetty-home/src/main/resources/bin/jetty.sh b/jetty-home/src/main/resources/bin/jetty.sh
index 9fd13d0281..c6882a543d 100755
--- a/jetty-home/src/main/resources/bin/jetty.sh
+++ b/jetty-home/src/main/resources/bin/jetty.sh
@@ -471,14 +471,14 @@ then
       do
         if [ -r "$XMLFILE" ] && [ -f "$XMLFILE" ]
         then
-          JETTY_ARGS=(${JETTY_ARGS[*]} "$XMLFILE")
+          JETTY_ARGS=(${JETTY_ARGS[*]} "'$XMLFILE'")
         else
           echo "** WARNING: Cannot read '$XMLFILE' specified in '$JETTY_CONF'"
         fi
       done
     else
       # assume it's a command line parameter (let start.jar deal with its validity)
-      JETTY_ARGS=(${JETTY_ARGS[*]} "$CONF")
+      JETTY_ARGS=(${JETTY_ARGS[*]} "'$CONF'")
     fi
   done < "$JETTY_CONF"
 fi
@@ -566,7 +566,7 @@ then
 fi
 
 # Collect the dry-run (of opts,path,main,args) from the jetty.base configuration
-JETTY_DRY_RUN=$("$JAVA" -jar "$JETTY_START" --dry-run=opts,path,main,args ${JETTY_ARGS[*]} ${JAVA_OPTIONS[*]})
+JETTY_DRY_RUN=$({ echo -jar "$JETTY_START" --dry-run=opts,path,main,args ; echo ${JETTY_ARGS[*]} ${JAVA_OPTIONS[*]} ; } | xargs "$JAVA" )
 RUN_ARGS=($JETTY_SYS_PROPS ${JETTY_DRY_RUN[@]})
 
 if (( DEBUG ))

Specifically I am adding single quotes around what I know are single arguments.
Then I'm using the xargs technique for running the dry-run command as well.

@joakime joakime moved this to 🏗 In progress in Jetty 12.0.3 - FROZEN Oct 12, 2023
@joakime joakime moved this to 🏗 In progress in Jetty 10.0.18 / 11.0.18 - FROZEN Oct 12, 2023
joakime added a commit that referenced this issue Oct 13, 2023
Signed-off-by: Joakim Erdfelt <[email protected]>
joakime added a commit that referenced this issue Oct 25, 2023
* Issue #10696 - Addressing start-stop-daemon behaviors in jetty.sh
* disable internal pid-file management of start-stop-daemon
* IssueDo not test for file system permissions if user is root, or process will switch to JETTY_USER
* Fixing bad UID / JETTY_USER condition
* Avoid FS test with setuid use as well
* Fixing stop behavior
* Adding jetty.sh docker testing

---------

Signed-off-by: Joakim Erdfelt <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Co-authored-by: Olivier Lamy <[email protected]>
@joakime
Copy link
Contributor Author

joakime commented Oct 25, 2023

Merged from 'jetty-10.0.x' to 'jetty-12.0.x'

@joakime joakime closed this as completed Oct 25, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 10.0.18 / 11.0.18 - FROZEN Oct 25, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.0.3 - FROZEN Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side High Priority
Projects
No open projects
Status: ✅ Done
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

4 participants