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

Fix null stream issue for resource loading based on relative names #816

Open
wants to merge 3 commits into
base: 9.0.x
Choose a base branch
from

Conversation

AmshikaH
Copy link

@AmshikaH AmshikaH commented Jan 31, 2025

This addresses a bug in the getResourceAsStream [1] method which falsely identifies resources to be absent when looking them up with relative paths if they have already been looked up with the absolute path.

In the getResourceAsStream [1] method, Tomcat checks the recently introduced notFoundClassResources based on the "path" variable alone, which is always the absolute path.

However, there are cases such as [2] in the Apache CXF core component where the class loading first tries calling the getResourceAsStream [1] method with the absolute path as the “name” and if this returns a null stream, it then tries the relative path by calling the getResourceAsStream [1] method with the relative path as the “name”.

In both calls to the getResourceAsStream method, the “path” variable set at [3] is the absolute path and therefore, Tomcat checks the cache for the absolute path both times.

As a result, the second attempt based on the relative path gets skipped, and a null stream is returned even though the resource is present.

In this PR, the block where the resource is loaded based on the "name" variable has been moved to a separate if block where the cache is checked for the "name" variable instead of the "path" variable.

[1]

public InputStream getResourceAsStream(String name) {

[2] https://github.com/apache/cxf/blob/cxf-3.5.9/core/src/main/java/org/apache/cxf/version/Version.java#L38
[3]
String path = nameToPath(name);

@markt-asf
Copy link
Contributor

getResourceAsStream() is documented to accept a name, not a URL. I'd like to understand the use case better before making any changes. Can you provide some sample code or a test case or something similar?

@AmshikaH
Copy link
Author

AmshikaH commented Feb 4, 2025

Sorry, I meant absolute and relative paths, not URLs. I have updated the description body to reflect this.

An example case would be the calling the getResourceAsStream method in Apache CXF Core's Version class (I use cxf-3.5.9) where the class loader is Tomcat's webapp class loader. The relevant snippet from Apache CXF Core's Version class is given below:

Screenshot 2025-02-04 at 16 33 40

In line 40, Apache CXF Core calls Tomcat's getResourceAsStream method by passing the absolute path /org/apache/cxf/version/version.properties as the name.

In line 42, Tomcat's getResourceAsStream method is called a second time by passing the relative path org/apache/cxf/version/version.properties as the name.

In both calls, the variables within Tomcat's getResourceAsStream are as follows.

  • First call:
    • name: /org/apache/cxf/version/version.properties (absolute)
    • path: /org/apache/cxf/version/version.properties (absolute)
  • Second call:
    • name: org/apache/cxf/version/version.properties (relative)
    • path: /org/apache/cxf/version/version.properties (absolute)

Regardless of whether the name variable is relative or absolute, the path variable is always absolute as it is derived using Tomcat's nameToPath method in Tomcat's getResourceAsStream method:

Screenshot 2025-02-04 at 17 06 43

Since the caching logic within Tomcat's getResourceAsStream only checks the path variable which is absolute, the second call based on the relative path gives a false positive that the resource has already been looked up and found to be unavailable with Tomcat 9.0.97 and 9.0.98.

@markt-asf
Copy link
Contributor

These are names not paths.

I'm not convinced that 'relative' and 'absolute' are concepts that apply here.

Putting that to one side, where is the target resource located so that the 'absolute' lookup fails but the 'relative' one works? Also, relative to what? What would be the 'absolute' path be that did work?

@AmshikaH
Copy link
Author

AmshikaH commented Feb 5, 2025

These are names not paths.

I'm not convinced that 'relative' and 'absolute' are concepts that apply here.

As per the Java documentation for resource names, "relative" and "absolute" do appear to be terminology that can be used in relation to a resource name, though not in the conventional sense.

However, let me re-phrase how I refer to them in my comments:

  • absolute name: a resource name that starts with a /
  • relative name: a resource name that does not start with a /

Putting that to one side, where is the target resource located so that the 'absolute' lookup fails but the 'relative' one works?

The resource is found within the cxf-core-3.5.9.jar as cxf-core-3.5.9.jar!/org/apache/cxf/version/version.properties.

The cxf jar is located outside the webapp in an external repository previously added via the addURL method in line 2536:

protected void addURL(URL url) {

When the cxf jar is present in an external repository, the 'absolute' lookup using the resource name that starts with a / fails and returns a null stream and the 'relative' lookup without the leading slash succeeds (for versions 9.0.96 and below) by getting the resource at line 1109 within the if block for external repositories:

URL url = super.findResource(name);

Also, relative to what?

My use of 'absolute' and 'relative' here have not been used in the conventional sense, but rather are used to indicate the presence of the leading slash similar to how it has been referred in the Java documentation for resource names.

However, if we were to use it in the conventional sense, I suppose it would be relative to the external repository?

What would be the 'absolute' path be that did work?

If this cxf jar is present in the webapp's WEB-INF/lib folder, the lookup with the 'absolute' name starting with / succeeds by getting the resource at line 1102 itself:

WebResource resource = resources.getClassLoaderResource(path);

@AmshikaH
Copy link
Author

AmshikaH commented Feb 5, 2025

Adding to my comment above, when the name variable passed is a name without the leading slash and the external repository lookup succeeds using this value, should we be failing the attempt based on a previous call where the name was a different value, i.e., with the slash, and preventing the non-slash lookup from executing?

This PR proposes updating the flow from checking only the path variable commonly prior to both lookups based on the path (line 1102) and name (line 1109) to checking for the path variable prior to the path based lookup, and checking for the name variable prior to the name based lookup. This also ensures backward compatibility.

@AmshikaH AmshikaH changed the title Fix null stream issue for resource loading based on relative paths Fix null stream issue for resource loading based on relative names Feb 6, 2025
@markt-asf
Copy link
Contributor

The Java documentation for resource names also states:

The methods in ClassLoader use the given String as the name of the resource without applying any absolute/relative transformation (see the methods in Class). The name should not have a leading "/".

That doesn't mean the issue isn't going to be fixed in Tomcat. But it does mean it looks like CXF has a bug that should be fixed.

@markt-asf
Copy link
Contributor

And given that addURL() is a protected method, how are you calling it?

@markt-asf
Copy link
Contributor

I think I understand everything that is going on now.

The root cause is that Tomcat accepts name for ClassLoader.getResource(String name) and friends with a leading '/' whereas the JRE provided class loaders do not.

My plan to deal with this is:

  • Tomcat 12 onwards return null if the name starts with /
  • Tomcat 11 and earlier, skip the notFoundClassResources cache if the name starts with '/'

Apps that use incorrect resource names may see a small performance degradation if they try loading the same resource multiple times using the wrong format for a resource name. The fix for that would be for the app to use the correct format.

@AmshikaH
Copy link
Author

AmshikaH commented Feb 6, 2025

But it does mean it looks like CXF has a bug that should be fixed.

Thank you for pointing this out. I will make a report of this via CXF's Jira board.

And given that addURL() is a protected method, how are you calling it?

We are using a custom class loader that extends Tomcat's WebappClassLoader, which makes our custom class loader a subclass of Tomcat's WebappClassLoaderBase.

  • Tomcat 12 onwards return null if the name starts with /
  • Tomcat 11 and earlier, skip the notFoundClassResources cache if the name starts with '/'

Apps that use incorrect resource names may see a small performance degradation if they try loading the same resource multiple times using the wrong format for a resource name. The fix for that would be for the app to use the correct format.

Thank you, a fix would be much appreciated.

@AmshikaH
Copy link
Author

AmshikaH commented Feb 6, 2025

The Java documentation for resource names also states:

The methods in ClassLoader use the given String as the name of the resource without applying any absolute/relative transformation (see the methods in Class). The name should not have a leading "/".

That doesn't mean the issue isn't going to be fixed in Tomcat. But it does mean it looks like CXF has a bug that should be fixed.

Please find the issue on CXF's Jira board here: https://issues.apache.org/jira/browse/CXF-9108

I have credited you by linking your GitHub profile and the quoted comment in the report. However, if you have an account in the Jira issue board that you would prefer being tagged instead, please do let me know and I can update the issue.

@AmshikaH
Copy link
Author

AmshikaH commented Feb 6, 2025

  • Tomcat 11 and earlier, skip the notFoundClassResources cache if the name starts with '/'

I have updated the PR according to this approach. Could you please check?

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

Successfully merging this pull request may close these issues.

2 participants