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

Issue #9511 Verify location of scanned class files #10777

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.StringTokenizer;
import java.util.concurrent.ConcurrentHashMap;

import org.eclipse.jetty.util.ExceptionUtil;
Expand Down Expand Up @@ -452,16 +453,18 @@ public AnnotationVisitor visitAnnotation(String desc, boolean visible)
public static class MyClassVisitor extends ClassVisitor
{
final int _asmVersion;
final Resource _containingResource;
final Resource _containingResource; //resource containing the class to parse
final Resource _classResource; //resource of the class being parsed
final Set<? extends Handler> _handlers;
ClassInfo _ci;

public MyClassVisitor(Set<? extends Handler> handlers, Resource containingResource, int asmVersion)
public MyClassVisitor(Set<? extends Handler> handlers, Resource containingResource, Resource classResource, int asmVersion)
{
super(asmVersion);
_asmVersion = asmVersion;
_handlers = handlers;
_containingResource = containingResource;
_classResource = classResource;
}

@Override
Expand All @@ -472,6 +475,14 @@ public void visit(final int version,
final String superName,
final String[] interfaces)
{
//Check that the named class exists in the containingResource at the correct location.
//eg given the class with name "com.foo.Acme" and the containingResource "jar:file://some/place/something.jar!/"
//then the file "jar:file://some/place/something.jar!/com/foo/Acme.class" must exist.
if (_containingResource != null && !checkClassContainment(_containingResource, _classResource, name))
{
throw new IllegalStateException("Class " + name + " not in correct location in " + _containingResource);
}

_ci = new ClassInfo(_containingResource, normalize(name), version, access, signature, normalize(superName), normalize(interfaces));
for (Handler h : _handlers)
{
Expand Down Expand Up @@ -520,6 +531,44 @@ public FieldVisitor visitField(final int access,
}
}

/**
* Check whether the classname is located inside its containingResource at
* the location that matches its package. The comparison accounts for classes
* located within WEB-INF/classes.
* @param containingResource the Resource that contains the class
* @param classResource the Resource representing the class
* @param classname the package-qualified name of the class
* @return true if the containingResource contains a class file at the location matching the package name of the class
*/
public static boolean checkClassContainment(Resource containingResource, Resource classResource, String classname)
{
if (containingResource == null || classResource == null)
return false;
Path relative = containingResource.getPathTo(classResource);
if (relative == null)
return false; //unable to be verified

StringTokenizer tokenizer = new StringTokenizer(classname, "/", false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the classname have '.' separators rather than '/'?

If it has '/' separators, then maybe we should split it with Path?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The classname when referenced via a resource has slashes.


for (int i = 0; i < relative.getNameCount(); i++)
{
String s = relative.getName(i).toString();
if (i == 0 && "WEB-INF".equalsIgnoreCase(s))
continue;
if (i == 1 && "classes".equalsIgnoreCase(s))
continue;
if (i == relative.getNameCount() - 1)
{
if ("class".equals(FileID.getExtension(s)))
s = s.substring(0, s.length() - 6);
}
Comment on lines +560 to +564
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment saying what this is doing would be good. something like // strip the .class suffix from the last path item.
Also, if the .class is missing then we should not match
Finally, it should be an equalsIgnoreCase

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be changed to ...

// strip class extensions
if (FileID.isExtension(s, "class"))
    s = s.substring(0, s.length() - 6)

As that will verify the extension in a case insensitive way.


if (!tokenizer.nextToken().equalsIgnoreCase(s))
return false;
}
return true;
}

public AnnotationParser()
{
this(ASM_VERSION);
Expand Down Expand Up @@ -564,7 +613,7 @@ public void parse(final Set<? extends Handler> handlers, Resource r) throws Exce

if (FileID.isClassFile(r.getPath()))
{
parseClass(handlers, null, r.getPath());
parseClass(handlers, null, r);
}

if (LOG.isDebugEnabled())
Expand Down Expand Up @@ -606,7 +655,7 @@ protected void parseDir(Set<? extends Handler> handlers, Resource dirResource) t

try
{
parseClass(handlers, dirResource, candidate.getPath());
parseClass(handlers, dirResource, candidate);
}
catch (Exception ex)
{
Expand Down Expand Up @@ -647,29 +696,28 @@ protected void parseJar(Set<? extends Handler> handlers, Resource jarResource) t
*
* @param handlers the handlers to look for classes in
* @param containingResource the dir or jar that the class is contained within, can be null if not known
* @param classFile the class file to parse
* @param classResource the class file to parse
* @throws IOException if unable to parse
*/
protected void parseClass(Set<? extends Handler> handlers, Resource containingResource, Path classFile) throws IOException
protected void parseClass(Set<? extends Handler> handlers, Resource containingResource, Resource classResource) throws IOException
{
if (LOG.isDebugEnabled())
LOG.debug("Parse class from {}", classFile.toUri());

URI location = classFile.toUri();
LOG.debug("Parse class from {}", classResource);

try (InputStream in = Files.newInputStream(classFile))
try (InputStream in = Files.newInputStream(classResource.getPath()))
{
ClassReader reader = new ClassReader(in);
reader.accept(new MyClassVisitor(handlers, containingResource, _asmVersion), ClassReader.SKIP_CODE | ClassReader.SKIP_DEBUG | ClassReader.SKIP_FRAMES);
reader.accept(new MyClassVisitor(handlers, containingResource, classResource, _asmVersion), ClassReader.SKIP_CODE | ClassReader.SKIP_DEBUG | ClassReader.SKIP_FRAMES);

String classname = normalize(reader.getClassName());
URI location = classResource.getPath().toUri();
URI existing = _parsedClassNames.putIfAbsent(classname, location);
if (existing != null)
LOG.warn("{} scanned from multiple locations: {}, {}", classname, existing, location);
}
catch (IllegalArgumentException | IOException e)
{
throw new IOException("Unable to parse class: " + classFile.toUri(), e);
throw new IOException("Unable to parse class: " + classResource.getURI(), e);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,11 @@
import java.util.concurrent.CopyOnWriteArrayList;

import org.eclipse.jetty.toolchain.test.FS;
import org.eclipse.jetty.toolchain.test.MavenPaths;
import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension;
import org.eclipse.jetty.util.IO;
import org.eclipse.jetty.util.TypeUtil;
import org.eclipse.jetty.util.resource.Resource;
import org.eclipse.jetty.util.resource.ResourceFactory;
Expand All @@ -46,10 +48,12 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.in;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;

@ExtendWith(WorkDirExtension.class)
public class TestAnnotationParser
Expand Down Expand Up @@ -187,21 +191,46 @@ public void handle(AnnotationParser.MethodInfo info, String annotation)
@Test
public void testHiddenFilesInJar() throws Exception
{
Path badClassesJar = MavenTestingUtils.getTestResourcePathFile("bad-classes.jar");
Path badClassesJar = MavenPaths.findTestResourceFile("bad-classes.jar");
AnnotationParser parser = new AnnotationParser();
Set<AnnotationParser.Handler> emptySet = Collections.emptySet();

try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
{
parser.parse(emptySet, resourceFactory.newResource(badClassesJar));
// only the valid classes inside bad-classes.jar should be parsed. If any invalid classes are parsed and exception would be thrown here
//Check class file in wrong location causes error
assertThrows(RuntimeException.class, () -> parser.parse(emptySet, resourceFactory.newResource(badClassesJar)));
}
//Check hidden and non classfiles skipped
assertThat(parser.getParsedClassNames().keySet(), containsInAnyOrder("Top", "com.acme.Foo"));
}

@Test
public void testHiddenAndBadFilesInDir() throws Exception
{
AnnotationParser parser = new AnnotationParser();
Set<AnnotationParser.Handler> emptySet = Collections.emptySet();

Path badClassesPath = MavenPaths.findTestResourceFile("bad-classes.jar");

try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
{
Path dir = MavenTestingUtils.getTargetTestingPath("baddir");
IO.delete(dir.toFile());
Resource badClassesJar = resourceFactory.newJarFileResource(badClassesPath.toUri());
badClassesJar.copyTo(dir);

//check class file in wrong location in jar causes error
assertThrows(RuntimeException.class, () -> parser.parse(emptySet, resourceFactory.newResource(dir)));
}

//Check hidden and non classfiles skipped
assertThat(parser.getParsedClassNames().keySet(), containsInAnyOrder("Top", "com.acme.Foo"));
}

@Test
public void testModuleInfoClassInJar() throws Exception
{
Path badClassesJar = MavenTestingUtils.getTestResourcePathFile("jdk9/slf4j-api-1.8.0-alpha2.jar");
Path badClassesJar = MavenPaths.findTestResourceFile("jdk9/slf4j-api-1.8.0-alpha2.jar");
AnnotationParser parser = new AnnotationParser();
Set<AnnotationParser.Handler> emptySet = Collections.emptySet();

Expand All @@ -210,12 +239,13 @@ public void testModuleInfoClassInJar() throws Exception
parser.parse(emptySet, resourceFactory.newResource(badClassesJar));
// Should throw no exceptions, and happily skip the module-info.class files
}
assertThat(parser.getParsedClassNames().keySet(), not(containsInAnyOrder("module-info")));
}

@Test
public void testJep238MultiReleaseInJar() throws Exception
{
Path badClassesJar = MavenTestingUtils.getTestResourcePathFile("jdk9/log4j-api-2.9.0.jar");
Path badClassesJar = MavenPaths.findTestResourceFile("jdk9/log4j-api-2.9.0.jar");
AnnotationParser parser = new AnnotationParser();
try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
{
Expand All @@ -234,7 +264,7 @@ public void testJep238MultiReleaseInJar() throws Exception
@Test
public void testJep238MultiReleaseInJarJDK10() throws Exception
{
Path jdk10Jar = MavenTestingUtils.getTestResourcePathFile("jdk10/multirelease-10.jar");
Path jdk10Jar = MavenPaths.findTestResourceFile("jdk10/multirelease-10.jar");
AnnotationParser parser = new AnnotationParser();

try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
Expand Down Expand Up @@ -286,8 +316,8 @@ public void testScanDuplicateClassesInJars() throws Exception
{
try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
{
Resource testJar = resourceFactory.newResource(MavenTestingUtils.getTargetPath("test-classes/tinytest.jar"));
Resource testJar2 = resourceFactory.newResource(MavenTestingUtils.getTargetPath("test-classes/tinytest_copy.jar"));
Resource testJar = resourceFactory.newResource(MavenPaths.findTestResourceFile("tinytest.jar"));
Resource testJar2 = resourceFactory.newResource(MavenPaths.findTestResourceFile("tinytest_copy.jar"));
AnnotationParser parser = new AnnotationParser();
DuplicateClassScanHandler handler = new DuplicateClassScanHandler();
Set<AnnotationParser.Handler> handlers = Collections.singleton(handler);
Expand All @@ -305,7 +335,7 @@ public void testScanDuplicateClasses() throws Exception
{
try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
{
Resource testJar = resourceFactory.newResource(MavenTestingUtils.getTargetFile("test-classes/tinytest.jar").toPath());
Resource testJar = resourceFactory.newResource(MavenPaths.findTestResourceFile("tinytest.jar"));
File testClasses = new File(MavenTestingUtils.getTargetDir(), "test-classes");
AnnotationParser parser = new AnnotationParser();
DuplicateClassScanHandler handler = new DuplicateClassScanHandler();
Expand Down
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
<Export-Package>org.example;version="${parsedVersion.majorVersion}.${parsedVersion.minorVersion}.${parsedVersion.incrementalVersion}"</Export-Package>
<Import-Package>javax.sql, jakarta.transaction;version="2.0.0"</Import-Package>
<_nouses>true</_nouses>
<Jetty-Environment>ee10</Jetty-Environment>
</instructions>
</configuration>
</plugin>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
<Provide-Capability>osgi.serviceloader; osgi.serviceloader=jakarta.servlet.ServletContainerInitializer</Provide-Capability>
<Export-Package>org.example.initializer;version="${parsedVersion.majorVersion}.${parsedVersion.minorVersion}.${parsedVersion.incrementalVersion}"</Export-Package>
<_nouses>true</_nouses>
<Jetty-Environment>ee10</Jetty-Environment>
</instructions>
</configuration>
</plugin>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public void parse(final Set<? extends Handler> handlers, Resource r) throws Exce

if (FileID.isClassFile(r.getPath()))
{
parseClass(handlers, null, r.getPath());
parseClass(handlers, null, r);
}

//Not already parsed, it could be a file that actually is compressed but does not have
Expand Down
6 changes: 6 additions & 0 deletions jetty-ee10/jetty-ee10-osgi/test-jetty-ee10-osgi/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@
<properties>
<assembly-directory>target/distribution</assembly-directory>
<bundle-symbolic-name>${project.groupId}.boot.test.osgi</bundle-symbolic-name>
<bundle.debug>false</bundle.debug>
<maven.deploy.skip>true</maven.deploy.skip>
<maven.javadoc.skip>true</maven.javadoc.skip>
<pax.exam.LEVEL>WARN</pax.exam.LEVEL>
<pax.exam.debug.port>0</pax.exam.debug.port>
</properties>
<dependencies>
<dependency>
Expand Down Expand Up @@ -519,7 +522,10 @@
<configuration>
<skipTests>${skipTests}</skipTests>
<systemPropertyVariables>
<bundle.debug>${bundle.debug}</bundle.debug>
<mavenRepoPath>${session.repositorySession.localRepository.basedir.absolutePath}</mavenRepoPath>
<pax.exam.debug.port>${pax.exam.debug.port}</pax.exam.debug.port>
<pax.exam.LEVEL>${pax.exam.LEVEL}</pax.exam.LEVEL>
<settingsFilePath>${env.GLOBAL_MVN_SETTINGS}</settingsFilePath>
</systemPropertyVariables>
<argLine>-Dconscrypt-version=${conscrypt.version}</argLine>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.eclipse.jetty.util.ssl.SslContextFactory;
import org.ops4j.pax.exam.CoreOptions;
import org.ops4j.pax.exam.Option;
import org.ops4j.pax.exam.options.extra.VMOption;
import org.ops4j.pax.tinybundles.core.TinyBundle;
import org.ops4j.pax.tinybundles.core.TinyBundles;
import org.osgi.framework.Bundle;
Expand Down Expand Up @@ -103,6 +104,11 @@ public static List<Option> configurePaxExamLogging()
options.add(systemProperty("pax.exam.logging").value("none"));
String paxExamLogLevel = System.getProperty("pax.exam.LEVEL", "WARN");
options.add(systemProperty("org.ops4j.pax.logging.DefaultServiceLog.level").value(paxExamLogLevel));
int debugPort = Integer.getInteger("pax.exam.debug.port", -1);
if (debugPort > 0)
{
options.add(new VMOption("-Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=" + debugPort));
}
return options;
}

Expand Down
Loading