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.

Sign up for GitHub

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

Jump to bottom

Guard against NullPointerException in AnnotationParser #12453

Open
guusdk opened this issue Oct 31, 2024 · 14 comments · May be fixed by #12454
Open

Guard against NullPointerException in AnnotationParser #12453

guusdk opened this issue Oct 31, 2024 · 14 comments · May be fixed by #12454
Assignees
@joakime
Labels
Bug For general bugs on Jetty side

Comments

@guusdk
Copy link

guusdk commented Oct 31, 2024 •
edited
Loading

We've got a broken setup, that is unlikely to be caused by the problem described herein. Via the stacktrace, however, we did find a possible improvement for Jetty 12.0.14 using EE8. No wars are going to be won by this, but still, it might be a useful improvement.

Given this stacktrace:

org.eclipse.jetty.ee8.webapp.WebAppContext - Failed startup of context oeje8w.WebAppContext@310ac970{ROOT,/,file:///home/guus/SourceCode/IgniteRealtime/Openfire/distribution/target/distribution-base/plugins/admin/webapp/,false}{/home/guus/SourceCode/IgniteRealtime/Openfire/distribution/target/distribution-base/plugins/admin/webapp}
java.lang.NullPointerException: Cannot invoke "java.nio.file.Path.getFileName()" because "path" is null
        at org.eclipse.jetty.util.FileID.isClassFile(FileID.java:369) ~[?:?]
        at org.eclipse.jetty.ee8.annotations.AnnotationParser.parse(AnnotationParser.java:489) ~[?:?]
        at org.eclipse.jetty.ee8.annotations.AnnotationConfiguration$ParserTask.call(AnnotationConfiguration.java:170) ~[?:?]
        at org.eclipse.jetty.util.ExceptionUtil.call(ExceptionUtil.java:333) ~[?:?]
        at org.eclipse.jetty.util.ExceptionUtil$MultiException.callAndCatch(ExceptionUtil.java:270) ~[?:?]
        at org.eclipse.jetty.ee8.annotations.AnnotationConfiguration.lambda$scanForAnnotations$0(AnnotationConfiguration.java:488) ~[?:?]
        at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:979) ~[?:?]
        at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1209) ~[?:?]
        at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1164) ~[?:?]
        at java.lang.Thread.run(Thread.java:840) [?:?]

It appears that there's room for improvement in AnnotationParser:

    public void parse(final Set<? extends Handler> handlers, Resource r) throws Exception {
        if (r == null)
            return;
        if (FileID.isJavaArchive(r.getPath())) {
            parseJar(handlers, r);
            return;
        }
        if (r.isDirectory()) {
            parseDir(handlers, r);
            return;
        }
        if (FileID.isClassFile(r.getPath())) {
            parseClass(handlers, null, r.getPath());
        }
        if (LOG.isDebugEnabled())
            LOG.warn("Resource not able to be scanned for classes: {}", r);
    }

If the Resource argument is null, the method immediately short-circuits to false. If the Resource is not null, but has a null path, then a NPE is thrown in only the last check (in FileID.isClassFile()). FileID.isJavaArchive() isn't bothered by having a null argument, but isClassFile is.

I'd suggest to make this behavior consistent, either by short-circuiting in the first check, or by adjusting the implementation of FileID.isClassFile()
@joakime
Copy link
Contributor

joakime commented Oct 31, 2024

What version of Jetty?

@guusdk
Copy link
Author

guusdk commented Oct 31, 2024

Jetty 12.0.14 using EE8

@joakime
Copy link
Contributor

joakime commented Oct 31, 2024

What Resource type are you using?
I'd like to setup a unit test for the same conditions.

@joakime
Copy link
Contributor

joakime commented Oct 31, 2024

Any fix for this wont make it into this months release. (too late for that)

@joakime joakime added Bug For general bugs on Jetty side and removed Enhancement labels Oct 31, 2024
@joakime joakime self-assigned this Oct 31, 2024
@guusdk
Copy link
Author

guusdk commented Oct 31, 2024

To be honest: I don't know yet. We're trying to do a forklift upgrade from 10.0.18 to 12.0.14 and everything is broken :). I've only just now started investigating why. This caught my eye and I'd thought to fire off a quick issue lest I'd forget.

I was not counting on a response within three minutes! /me waves angry first :)

joakime added a commit that referenced this issue Oct 31, 2024
@joakime
Issue #12453 - AnnotationParser NPE protection
f1905e6 
+ Better utilize Resource object, don't rely
  on Path existing for things that don't
  require Path.
@joakime
Copy link
Contributor

joakime commented Oct 31, 2024 •
edited
Loading

@guusdk a WIP PR for this is already opened at #12454

It's missing a testcase, something to prevent regression.
It would help if you can put a breakpoint in that AnnotationPaser.parse() method.
Stopping if r.getPath() is null.
Then telling us what your r (the Resource object) type is.
Even the details of that Resource object, as a screenshot of your debug window or a copy/paste would be helpful.

@joakime joakime linked a pull request Oct 31, 2024 that will close this issue
Issue #12453 - Allow AnnotationParser to work with Resources types that do not support Path #12454
Draft
@guusdk
Copy link
Author

guusdk commented Oct 31, 2024

The offender seems to be an instance of org.eclipse.jetty.util.resource.URLResourceFactory.URLResource

image

@joakime
Copy link
Contributor

joakime commented Oct 31, 2024

Looks like OpenFire is setting the scheme file to the URLResourceFactory. That's not a great idea, it will cause all kinds of strange file locking issues with the deprecated (in the JVM) URL classes.

@joakime
Copy link
Contributor

joakime commented Oct 31, 2024

Huh, OpenFire seems to only support Jetty 10.0.x
Are you upgrading it?

@joakime
Copy link
Contributor

joakime commented Oct 31, 2024

Ya, looks like you are developer with OpenFire.
Do you have a branch that you are doing this work in?

Don't set / use the URLResourceFactory for the default schemes (file, jrt, jar, etc).
If you build against graalvm, definitely do not use URLResourceFactory, stick with the defaults.

Only use it for oddball schemes like bundle (found in OSGi land).

@guusdk
Copy link
Author

guusdk commented Oct 31, 2024

Yeah, like I wrote earlier, we ran into this while working to get Jetty upgraded in our project.

Thanks for the pointers!

@guusdk
Copy link
Author

guusdk commented Oct 31, 2024

@joakime The upgrade is being worked on here: https://github.com/surevine/Openfire/tree/feature/jetty-12.0.14-ee8-upgrade

What we're upgrading is old code that itself might not have been in the best state. We've been using Jetty for ages. Most of the upgrades were a 'hit it until it runs' kind of effort.

@joakime
Copy link
Contributor

joakime commented Oct 31, 2024

I tossed a few quick comments in your commits that might help you.

@guusdk
Copy link
Author

guusdk commented Oct 31, 2024

That's super helpful, thanks!

@janbartel janbartel removed their assignment Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@joakime joakime

Labels
Bug For general bugs on Jetty side
Projects
Jetty 12.0.16
Status: No status
Milestone
No milestone
3 participants
@joakime @janbartel @guusdk