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

Improve Working Directory creation #5451

Closed
joakime opened this issue Oct 15, 2020 · 10 comments · Fixed by #5453
Closed

Improve Working Directory creation #5451

joakime opened this issue Oct 15, 2020 · 10 comments · Fixed by #5453
Assignees

Comments

@joakime
Copy link
Contributor

joakime commented Oct 15, 2020

Jetty version
9.4.x

Java version
All

OS type/version
All

Description
When the WebInfConfiguration deploys an application, the temp directory creation is using an old Java Classlib API for creating unique directories.

Lets modernize this, as newer Java Classlib APIs exist to make this a single step.

@joakime joakime self-assigned this Oct 15, 2020
joakime added a commit that referenced this issue Oct 15, 2020
+ Using new Files.createTempDirectory() instead
  of nonsense around File.createTempFile()

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime changed the title Improve Workiing Directory creation Improve Working Directory creation Oct 15, 2020
@sbordet
Copy link
Contributor

sbordet commented Oct 15, 2020

@joakime there are 22 usages of File.createTempFile(), mostly in tests, but also in a few other spots in the code.
Care to fix them all at this point?

@joakime
Copy link
Contributor Author

joakime commented Oct 15, 2020

The focus of this was Temp/Work Directories, not Temp/Work Files.

I was focusing this PR on those usages of File.createTempFile() that were using it to establish unique directory names (as opposed to unique file names).
There's only 2 of those scenarios I could find.

@sbordet
Copy link
Contributor

sbordet commented Oct 15, 2020

But why not fixing them all?

@joakime
Copy link
Contributor Author

joakime commented Oct 15, 2020

But why not fixing them all?

It's a bigger change then you would expect.
Take this one for example ...

https://github.com/eclipse/jetty.project/blob/de97d26f7bd222a0e16831e353d702a7a422f711/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java#L198-L200

This just causes a no-op on Windows, but works on Linux, and causes logging exceptions on OSX.
If I change that to use java.nio.files.Path concepts the code gets complicated, quickly.
As I now use the FileAttributes on the Files.createTempFile()

https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/Files.html#createTempFile(java.nio.file.Path,java.lang.String,java.lang.String,java.nio.file.attribute.FileAttribute...)

That means I need to interrogate the FileSytem to know what FileAttributes I can use (Posix, Osx, Windows, NT, etc...) to manage the attributes on the created file appropriately.
Even if we just limit it to Posix and Dos (not NT), we have a great deal of code to figure out the attributes appropriately.

Those 3 lines of code will quickly grow.

Example (from a long ago discarded PR) :
https://github.com/eclipse/jetty.project/blob/388eb138bb244479560f6b7a63f2812b93f58320/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java#L244-L319

@joakime joakime linked a pull request Oct 15, 2020 that will close this issue
@joakime
Copy link
Contributor Author

joakime commented Oct 15, 2020

I'll update the PR, but I'm not going to try to fix ...

 _file.setReadable(false, WORLD); // (reset) disable it for everyone first 
 _file.setReadable(true, USER); // enable for user only 

as it gets too complicated outside of unix. (i really don't want to deal with ACL)

joakime added a commit that referenced this issue Oct 15, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Oct 15, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@olamy
Copy link
Member

olamy commented Oct 16, 2020

still this PR #5453 to merge

@olamy olamy reopened this Oct 16, 2020
joakime added a commit that referenced this issue Oct 16, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Oct 16, 2020
…r-cleanup

Issue #5451 - Cleanup of temp file usages.
@joakime joakime linked a pull request Oct 16, 2020 that will close this issue
@joakime
Copy link
Contributor Author

joakime commented Oct 16, 2020

PR #5453 merged.

@joakime joakime closed this as completed Oct 16, 2020
joakime added a commit that referenced this issue Oct 16, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Oct 16, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Oct 16, 2020
… directory.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Oct 16, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Oct 16, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Oct 17, 2020
…anup

Issue #5451 - Removing file/dir permission management from codebase
joakime added a commit that referenced this issue Oct 19, 2020
* Issue #5451 - Improving temp directory creation.

+ Using new Files.createTempDirectory() instead
  of nonsense around File.createTempFile()

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

* Fixes #5451 - Restoring File.deleteOnExit
@suzith
Copy link

suzith commented Nov 5, 2020

I recently upgraded to jetty-9.4.32.v20200930 and intermittently getting following exception while accessing jsp for the first time :
org.apache.jasper.JasperException: Unable to compile class for JSP
at org.apache.jasper.JspCompilationContext.compile(JspCompilationContext.java:612) ~[org.mortbay.jasper.apache-jsp-8.5.54.jar:8.5.54]
at org.apache.jasper.servlet.JspServletWrapper.service(JspServletWrapper.java:399) ~[org.mortbay.jasper.apache-jsp-8.5.54.jar:8.5.54]
at org.apache.jasper.servlet.JspServlet.serviceJspFile(JspServlet.java:386) ~[org.mortbay.jasper.apache-jsp-8.5.54.jar:8.5.54]
...
...
...
Caused by: java.io.IOException: Unable to delete class file [XXXXXX_jsp.class]
at org.apache.jasper.compiler.SmapUtil$SDEInstaller.install(SmapUtil.java:201) ~[org.mortbay.jasper.apache-jsp-8.5.54.jar:8.5.54]
at org.apache.jasper.compiler.SmapUtil.installSmap(SmapUtil.java:163) ~[org.mortbay.jasper.apache-jsp-8.5.54.jar:8.5.54]
at org.apache.jasper.compiler.JDTCompiler.generateClass(JDTCompiler.java:589) ~[org.mortbay.jasper.apache-jsp-8.5.54.jar:8.5.54]
at org.apache.jasper.compiler.Compiler.compile(Compiler.java:381) ~[org.mortbay.jasper.apache-jsp-8.5.54.jar:8.5.54]
at org.apache.jasper.compiler.Compiler.compile(Compiler.java:351) ~[org.mortbay.jasper.apache-jsp-8.5.54.jar:8.5.54]
at org.apache.jasper.compiler.Compiler.compile(Compiler.java:335) ~[org.mortbay.jasper.apache-jsp-8.5.54.jar:8.5.54]
at org.apache.jasper.JspCompilationContext.compile(JspCompilationContext.java:597) ~[org.mortbay.jasper.apache-jsp-8.5.54.jar:8.5.54]

Is my issue related to this issue (#5451) ?

@edhn3000
Copy link

edhn3000 commented Aug 16, 2022

is this bug fixed in 9.3.x? I found #5451 commit in 9.3.29.v20201019, but the bug fix report(CVE-2020-27216) and this page both has not say about it, https://github.com/eclipse/jetty.project/security/advisories/GHSA-g3wg-6mcf-8jj6#advisory-comment-63053
so can I use any version in 9.3.x to fix it?
because when we upgrade jetty from 9.3.27.v20190418 to 9.4.33.v20201020, we got some error

@joakime
Copy link
Contributor Author

joakime commented Aug 16, 2022

@edhn3000 check the VERSION.txt

https://github.com/eclipse/jetty.project/blob/jetty-9.3.30.v20211001/VERSION.txt

Know that Jetty 9.3.x was EOL in early 2020 - https://www.eclipse.org/lists/jetty-announce/msg00140.html
So that's why it's not mentioned on the CVE from November 2020. (As is practice in CVE reporting, Versions that are no longer supported are not mentioned)

Also, Jetty 9.4.x is now at End of Community Support - #7958

because when we upgrade jetty from 9.3.27.v20190418 to 9.4.33.v20201020, we got some error

9.4.33.v20201020 would be a poor version to upgrade to, as it's subject to other security issues.
See https://www.eclipse.org/jetty/security_reports.php

Reminder: Jetty versioning (since 1995) has been <servlet-support>.<major>.<minor> so going from 9.3 to 9.4 is considered a major version upgrade.

You should be upgrading to Jetty 10.x at least at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants