jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.83k stars 1.91k forks source link

JSP temp directory regression, possibly due to fix for #12044 #12124

Closed scantor closed 1 month ago

scantor commented 1 month ago

The 12.0.12 patch for issue 12044 seems to have possibly caused a regression in the JSP temp folder creation step.

Starting 12.0.12 I get this: Caused by: java.lang.IllegalStateException: Could not create JSP scratch directory

I can see on 12.0.11 I get a jsp folder under "tmp/jetty-0_0_0_0-80-idp_war-_idp-any-4380240850460342364"

On 12.0.12, it can't create it. I think the internal name of the tmp folder is being munged so by the time it goes to create the jsp folder, it isn't using the right path. I noticed I think an extra hyphen or possibly some other stray character in the folder name it created, so something seems to have gone wrong somewhere, like the folder created on disk doesn't match the internal variable holding the folder path it defaults the jsp path to.

joakime commented 1 month ago

Are you using Windows? What JVM?

Please don't delete the issue template next time.

scantor commented 1 month ago

It's Linux, Coretto 17.

There is some anecdotal evidence it may not affect the Windows version, I don't have it to test fully.

In my case, it was a straight upgrade (stop 0.11, update my init script pointer, start 0.12) resulting in the exception in the log. If nothiing obvious shows up, once I'm done with some other work I can do a bit more digging tomorrow and compare the tmp folders I'm getting between the two versions, I am 99% sure they were subtly different.

joakime commented 1 month ago

Are you manually configuring the temp directory? Either at the server level, or at the context level, or at the webapp level?

joakime commented 1 month ago

Are you using a classic jetty-home / jetty-base setup? Or custom jetty-home / jetty-base? Or embedded? When this occurs is it happening on a normal command line? or docker? If using docker, are you using the official jetty.docker images for Coretto 17 and Jetty 12?

scantor commented 1 month ago

I am configuring the temp directory with an ini file via -Djava.io.tmpdir=tmp (located in my jetty-base), and the top level webapp and nested webinf folder are created without any problems. On 12.0.11, it also creates the nested jsp subfolder, whereas on 12.0.12, that step fails, taking down the webapp.

I'm not sure what constitutes "classic", but I have jetty-home in /opt, just unpacking the distribution as posted, and my jetty-base is elsewhere, and my init script passes in the necessary variables to point it to them.

Nothing embedded, no.

My startup is via an init script that's fairly exotic due to use of setuid (working fine) and not having managed to get a systemd unit file to work so I do some crazy --dry-run stuff that has been working fine with 12.0.11, unchanged from Jetty 11.

Permissions all seem fine, evidenced by the fact that it's able to create all the other material in tmp.

Red Hat 9 is the only place I've specifically reproduced the bug.

No Docker whatsoever.

If not for the fact that my colleague reported Windows as working, I would be certain this is a regression from the cleanup fix. I'm still close to certain, simply from Occam's Razor.

scantor commented 1 month ago

If it helps, this is what 12.0.11 looks like. I can get a sample of the partial pre-crash content on 12.0.12 tomorrow latest.

[shibboleth@otdi-ssoqa01 ~]$ ls -al jetty/tmp
total 4
drwxrwx---.  6 shibboleth shibboleth 4096 Aug  1 14:49 .
drwxr-x---. 11 shibboleth shibboleth  132 Jul 15 10:52 ..
drwxrwx---.  4 jetty      shibboleth   31 Aug  1 14:49 jetty-0_0_0_0-80-idp_war-_idp-any-7687450141413216206

[shibboleth@otdi-ssoqa01 ~]$ ls -al jetty/tmp/jetty-0_0_0_0-80-idp_war-_idp-any-7687450141413216206/
total 4
drwxrwx---. 4 jetty      shibboleth   31 Aug  1 14:49 .
drwxrwx---. 6 shibboleth shibboleth 4096 Aug  1 14:49 ..
drwxrwx---. 3 jetty      shibboleth   17 Aug  1 14:50 jsp
drwxrwx---. 3 jetty      shibboleth   21 Aug  1 14:49 webinf
joakime commented 1 month ago

so I do some crazy --dry-run stuff that has been working fine with 12.0.11, unchanged from Jetty 11.

Note that there are few more <parts> available on --dry-run=<part>,<part> on Jetty 12 that might be useful to you. See start.jar --help for details.

joakime commented 1 month ago

The "jsp" subdirectory is the Apache Jasper "scratchdir".

Internally, on startup of the ServletContext, specifically the JspServlet, the scratchdir init-param is looked for, if it doesn't exist, it uses ...

https://github.com/jetty/jetty.project/blob/cc6f1b74db755fed228b50701ad967aeaa68e83f/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHolder.java#L681-L692

So if the temp directory doesn't exist, the call File.mkdir() will fail.

scantor commented 1 month ago

Right. My impression is that the defaulted in scratchdir built from line 685-687 is not in fact the same path as what's been placed on disk previously, so the folder in the middle of the path doesn't exist.

It wasn't obvious where I should define my own scratchdir if that were a workaround, but I don't know that I'd want to go that route anyway. If it would have to be in my actual webapp's servlet declarations, that's "non-ideal" for sure.

joakime commented 1 month ago

the jsp servlet definition, typically seen in etc/webdefault-ee10.xml would be the place to add/change arbitrary init-param stuff.

Just copy the one from ${jetty.home/etc/ into your ${jetty.base}/etc/ and edit the copied one.

https://github.com/jetty/jetty.project/blob/58cfe7709fbcea5cbcaa389bfe52f0470a253ad7/jetty-ee10/jetty-ee10-webapp/src/main/config/etc/webdefault-ee10.xml#L181-L207

scantor commented 1 month ago

Ok, I'm in the midst of my main upgrade to 12.0.11 so once I get back to ground zero I can do the comparison and also try some workarounds. I have feelers out to others who can refute or reproduce my results.

pbhenson commented 1 month ago

I tested this, and it appears that 12.0.12 creates the jsp temp directory in jetty_base rather than in the specific tmpdir for the webapp in question:

# find /var/lib/jetty/jsp
/var/lib/jetty/jsp
/var/lib/jetty/jsp/org
/var/lib/jetty/jsp/org/apache
/var/lib/jetty/jsp/org/apache/jsp
/var/lib/jetty/jsp/org/apache/jsp/WEB_002dINF
/var/lib/jetty/jsp/org/apache/jsp/WEB_002dINF/jsp
/var/lib/jetty/jsp/org/apache/jsp/WEB_002dINF/jsp/metadata_jsp.java
/var/lib/jetty/jsp/org/apache/jsp/WEB_002dINF/jsp/metadata_jsp.class
/var/lib/jetty/jsp/org/apache/jsp/WEB_002dINF/jsp/status_jsp.java
/var/lib/jetty/jsp/org/apache/jsp/WEB_002dINF/jsp/status_jsp.class
/var/lib/jetty/jsp/org/apache/jsp/index_jsp.java
/var/lib/jetty/jsp/org/apache/jsp/index_jsp.class

Scott's jetty service account doesn't have write access to jetty_base, which is presumably why he's getting that error.

Also, it appears that regardless of whether persistTempDirectory is not specified, is set to true, or is set to false, the webapp temporary directory files are always deleted. Although the jsp tmpdir in the wrong location remains...

This is definitely a change in behavior from earlier versions, and I don't think having to go manually change parameters is the right answer to fixing it. Also, while I'm only running one webapp on this server, it seems if multiple jsp webapps were in place they'd be sharing a temporary directory which seems inadvisable and possibly a security issue. If you can confirm this, you might want to pull this version.

Thanks...

janbartel commented 1 month ago

@scantor @pbhenson next time (if there is a next time) could you please include all of the information on the issue template? Most particularly it is critical for us to know which EE environment you are using. As neither of you gave the environment, I've assumed ee10 and tested that first, and can confirm there is no problem. On a hunch, I tested ee9, and it seems that the problem lies there - it would have been better to know that up front.

Investigating now.

janbartel commented 1 month ago

OK, found the problem. I've raised #12129 to fix.

As a workaround in the meanwhile, you could try explicitly setting the location of the tmp directory for each webapp (WebAppContext.setTempDirectory(File).

scantor commented 1 month ago

Thank you. I didn't realize the extent to which the implementation was that distinct across EE versions, that seems likely to be a maintenance fiasco. Never occured to me it might impact the behavior.