This is an automated email from the ASF dual-hosted git repository.
pauls pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/felix-dev.git
The following commit(s) were added to refs/heads/master by this push:
new c2da9a5964 Felix-6535: Cleanup directory revision exception handling
(#154)
c2da9a5964 is described below
commit c2da9a5964c5a9e0132e897d50f93a703e6b2f93
Author: Karl Pauls <[email protected]>
AuthorDate: Fri May 20 00:30:29 2022 +0200
Felix-6535: Cleanup directory revision exception handling (#154)
---
.../felix/framework/cache/DirectoryContent.java | 147 +++++++++++++++------
.../felix/framework/cache/BundleCacheTest.java | 13 +-
2 files changed, 116 insertions(+), 44 deletions(-)
diff --git
a/framework/src/main/java/org/apache/felix/framework/cache/DirectoryContent.java
b/framework/src/main/java/org/apache/felix/framework/cache/DirectoryContent.java
index c462b5539f..3e4a2ad934 100644
---
a/framework/src/main/java/org/apache/felix/framework/cache/DirectoryContent.java
+++
b/framework/src/main/java/org/apache/felix/framework/cache/DirectoryContent.java
@@ -27,6 +27,7 @@ import org.osgi.framework.Constants;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
+import java.io.UncheckedIOException;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.Enumeration;
@@ -48,9 +49,10 @@ public class DirectoryContent implements Content
private final File m_rootDir;
private final File m_dir;
private Map m_nativeLibMap;
+ private final String m_canonicalRoot;
public DirectoryContent(Logger logger, Map configMap,
- WeakZipFileFactory zipFactory, Object revisionLock, File rootDir, File
dir)
+ WeakZipFileFactory zipFactory, Object
revisionLock, File rootDir, File dir)
{
m_logger = logger;
m_configMap = configMap;
@@ -58,6 +60,16 @@ public class DirectoryContent implements Content
m_revisionLock = revisionLock;
m_rootDir = rootDir;
m_dir = dir;
+ String canonicalPath = null;
+ try {
+ canonicalPath =
BundleCache.getSecureAction().getCanonicalPath(m_dir);
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ if (!canonicalPath.endsWith(File.separator)) {
+ canonicalPath = canonicalPath + "/";
+ }
+ m_canonicalRoot = canonicalPath;
}
public File getFile()
@@ -77,9 +89,17 @@ public class DirectoryContent implements Content
// Return true if the file associated with the entry exists,
// unless the entry name ends with "/", in which case only
// return true if the file is really a directory.
- File file = new File(m_dir, name);
+ File file = null;
+ try
+ {
+ file = getFile(name);
+ }
+ catch (IOException e)
+ {
+ return false;
+ }
return BundleCache.getSecureAction().fileExists(file)
- && (name.endsWith("/")
+ && (name.endsWith("/")
? BundleCache.getSecureAction().isFileDirectory(file) : true);
}
@@ -91,7 +111,15 @@ public class DirectoryContent implements Content
// Return true if the file associated with the entry exists,
// unless the entry name ends with "/", in which case only
// return true if the file is really a directory.
- File file = new File(m_dir, name);
+ File file = null;
+ try
+ {
+ file = getFile(name);
+ }
+ catch (IOException e)
+ {
+ return false;
+ }
return BundleCache.getSecureAction().isFileDirectory(file);
}
@@ -110,36 +138,36 @@ public class DirectoryContent implements Content
// Get the embedded resource.
- File file = new File(m_dir, name);
try
{
+ File file = getFile(name);
return BundleCache.getSecureAction().fileExists(file) ?
BundleCache.read(BundleCache.getSecureAction().getInputStream(file),
file.length()) : null;
}
catch (Exception ex)
{
m_logger.log(
- Logger.LOG_ERROR,
- "DirectoryContent: Unable to read bytes for file " + name + "
from file " + file.getAbsolutePath(), ex);
+ Logger.LOG_ERROR,
+ "DirectoryContent: Unable to read bytes for file " + name
+ " from file " + new File(m_dir, name).getAbsolutePath(), ex);
return null;
}
}
public InputStream getEntryAsStream(String name)
- throws IllegalStateException, IOException
+ throws IllegalStateException, IOException
{
name = getName(name);
- File file = new File(m_dir, name);
try
{
+ File file = getFile(name);
return BundleCache.getSecureAction().fileExists(file) ?
BundleCache.getSecureAction().getInputStream(file) : null;
}
catch (Exception ex)
{
m_logger.log(
- Logger.LOG_ERROR,
- "DirectoryContent: Unable to create inputstream for file " +
name + " from file " + file.getAbsolutePath(), ex);
+ Logger.LOG_ERROR,
+ "DirectoryContent: Unable to create inputstream for file "
+ name + " from file " + new File(m_dir, name).getAbsolutePath(), ex);
return null;
}
}
@@ -152,6 +180,22 @@ public class DirectoryContent implements Content
return name;
}
+ private File getFile(String name) throws IOException
+ {
+ File result = new File(m_dir, name);
+
+ String canonicalPath =
BundleCache.getSecureAction().getCanonicalPath(result);
+ if (BundleCache.getSecureAction().isFileDirectory(result) &&
!canonicalPath.endsWith(File.separator))
+ {
+ canonicalPath += File.separator;
+ }
+ if (!canonicalPath.startsWith(m_canonicalRoot))
+ {
+ throw new IOException("File outside the root: " + canonicalPath);
+ }
+ return result;
+ }
+
public URL getEntryAsURL(String name)
{
name = getName(name);
@@ -160,9 +204,9 @@ public class DirectoryContent implements Content
{
try
{
- return BundleCache.getSecureAction().toURI(new File(m_dir,
name)).toURL();
+ return
BundleCache.getSecureAction().toURI(getFile(name)).toURL();
}
- catch (MalformedURLException e)
+ catch (IOException e)
{
return null;
}
@@ -178,7 +222,12 @@ public class DirectoryContent implements Content
{
name = getName(name);
- File file = new File(m_dir, name);
+ File file = null;
+ try {
+ file = getFile(name);
+ } catch (IOException e) {
+ return 0L;
+ }
return BundleCache.getSecureAction().getLastModified(file);
}
@@ -190,7 +239,7 @@ public class DirectoryContent implements Content
if (entryName.equals(FelixConstants.CLASS_PATH_DOT))
{
return new DirectoryContent(
- m_logger, m_configMap, m_zipFactory, m_revisionLock,
m_rootDir, m_dir);
+ m_logger, m_configMap, m_zipFactory, m_revisionLock,
m_rootDir, m_dir);
}
// Remove any leading slash, since all bundle class path
@@ -198,9 +247,9 @@ public class DirectoryContent implements Content
entryName = (entryName.startsWith("/")) ? entryName.substring(1) :
entryName;
if (entryName.trim().startsWith(".." + File.separatorChar) ||
- entryName.contains(File.separator + ".." + File.separatorChar) ||
- entryName.trim().endsWith(File.separator + "..") ||
- entryName.trim().equals(".."))
+ entryName.contains(File.separator + ".." + File.separatorChar)
||
+ entryName.trim().endsWith(File.separator + "..") ||
+ entryName.trim().equals(".."))
{
return null;
}
@@ -208,33 +257,38 @@ public class DirectoryContent implements Content
// Any embedded JAR files will be extracted to the embedded directory.
File embedDir = new File(m_rootDir, m_dir.getName() +
EMBEDDED_DIRECTORY);
- // Determine if the entry is an emdedded JAR file or
+ // Determine if the entry is an embedded JAR file or
// directory in the bundle JAR file. Ignore any entries
// that do not exist per the spec.
- File file = new File(m_dir, entryName);
+ File file = null;
+ try {
+ file = getFile(entryName);
+ } catch (IOException e) {
+ return null;
+ }
if (BundleCache.getSecureAction().isFileDirectory(file))
{
return new DirectoryContent(
- m_logger, m_configMap, m_zipFactory, m_revisionLock,
m_rootDir, file);
+ m_logger, m_configMap, m_zipFactory, m_revisionLock,
m_rootDir, file);
}
else if (BundleCache.getSecureAction().fileExists(file)
- && entryName.endsWith(".jar"))
+ && entryName.endsWith(".jar"))
{
File extractDir = new File(embedDir,
- (entryName.lastIndexOf('/') >= 0)
- ? entryName.substring(0, entryName.lastIndexOf('/'))
- : entryName);
+ (entryName.lastIndexOf('/') >= 0)
+ ? entryName.substring(0,
entryName.lastIndexOf('/'))
+ : entryName);
return new JarContent(
- m_logger, m_configMap, m_zipFactory, m_revisionLock,
- extractDir, file, null);
+ m_logger, m_configMap, m_zipFactory, m_revisionLock,
+ extractDir, file, null);
}
// The entry could not be found, so return null.
return null;
}
-// TODO: SECURITY - This will need to consider security.
+ // TODO: SECURITY - This will need to consider security.
public String getEntryAsNativeLibrary(String entryName)
{
// Return result.
@@ -245,9 +299,9 @@ public class DirectoryContent implements Content
entryName = (entryName.startsWith("/")) ? entryName.substring(1) :
entryName;
if (entryName.trim().startsWith(".." + File.separatorChar) ||
- entryName.contains(File.separator + ".." + File.separatorChar) ||
- entryName.trim().endsWith(File.separator + "..") ||
- entryName.trim().equals(".."))
+ entryName.contains(File.separator + ".." + File.separatorChar)
||
+ entryName.trim().endsWith(File.separator + "..") ||
+ entryName.trim().equals(".."))
{
return null;
}
@@ -257,9 +311,18 @@ public class DirectoryContent implements Content
// The entry must exist and refer to a file, not a directory,
// since we are expecting it to be a native library.
- File entryFile = new File(m_dir, entryName);
+ File entryFile = null;
+ try
+ {
+ entryFile = getFile(entryName);
+ }
+ catch (IOException e)
+ {
+ return null;
+ }
+
if (BundleCache.getSecureAction().fileExists(entryFile)
- && !BundleCache.getSecureAction().isFileDirectory(entryFile))
+ && !BundleCache.getSecureAction().isFileDirectory(entryFile))
{
// Extracting the embedded native library file impacts all other
// existing contents for this revision, so we have to grab the
@@ -279,16 +342,16 @@ public class DirectoryContent implements Content
libCount = (libCount == null) ? new Integer(0) : new
Integer(libCount.intValue() + 1);
m_nativeLibMap.put(entryName, libCount);
File libFile = new File(
- libDir, libCount.toString() + File.separatorChar +
entryName);
+ libDir, libCount.toString() + File.separatorChar +
entryName);
if (!BundleCache.getSecureAction().fileExists(libFile))
{
if
(!BundleCache.getSecureAction().fileExists(libFile.getParentFile())
- &&
!BundleCache.getSecureAction().mkdirs(libFile.getParentFile()))
+ &&
!BundleCache.getSecureAction().mkdirs(libFile.getParentFile()))
{
m_logger.log(
- Logger.LOG_ERROR,
- "Unable to create library directory.");
+ Logger.LOG_ERROR,
+ "Unable to create library directory.");
}
else
{
@@ -304,7 +367,7 @@ public class DirectoryContent implements Content
// Perform exec permission command on extracted
library
// if one is configured.
String command = (String) m_configMap.get(
- Constants.FRAMEWORK_EXECPERMISSION);
+ Constants.FRAMEWORK_EXECPERMISSION);
if (command != null)
{
Properties props = new Properties();
@@ -320,8 +383,8 @@ public class DirectoryContent implements Content
catch (Exception ex)
{
m_logger.log(
- Logger.LOG_ERROR,
- "Extracting native library.", ex);
+ Logger.LOG_ERROR,
+ "Extracting native library.", ex);
}
}
}
@@ -367,7 +430,7 @@ public class DirectoryContent implements Content
// Convert the file separator character to slashes.
String abs = BundleCache.getSecureAction()
-
.getAbsolutePath(m_children[m_counter]).replace(File.separatorChar, '/');
+
.getAbsolutePath(m_children[m_counter]).replace(File.separatorChar, '/');
// Remove the leading path of the reference directory, since the
// entry paths are supposed to be relative to the root.
@@ -398,7 +461,7 @@ public class DirectoryContent implements Content
File[] tmp = new File[combined.length +
grandchildren.length];
System.arraycopy(combined, 0, tmp, 0,
combined.length);
System.arraycopy(
- grandchildren, 0, tmp, combined.length,
grandchildren.length);
+ grandchildren, 0, tmp, combined.length,
grandchildren.length);
combined = tmp;
}
}
diff --git
a/framework/src/test/java/org/apache/felix/framework/cache/BundleCacheTest.java
b/framework/src/test/java/org/apache/felix/framework/cache/BundleCacheTest.java
index 5dbfb74ef3..793334d81a 100644
---
a/framework/src/test/java/org/apache/felix/framework/cache/BundleCacheTest.java
+++
b/framework/src/test/java/org/apache/felix/framework/cache/BundleCacheTest.java
@@ -152,7 +152,14 @@ public class BundleCacheTest extends TestCase
public void testDirectoryReference() throws Exception
{
- testBundle("reference:" + archiveFile.toURI().toURL(), null);
+ BundleArchive archive = testBundle("reference:" +
archiveFile.toURI().toURL(), null);
+ String path =
"../../../../../../../../../../../../../../../../..".replace("/",
File.separator);
+ assertFalse(archive.getCurrentRevision().getContent().hasEntry(path));
+
assertFalse(archive.getCurrentRevision().getContent().isDirectory(path));
+
assertNull(archive.getCurrentRevision().getContent().getEntryAsURL(path));
+
assertNull(archive.getCurrentRevision().getContent().getEntryAsBytes(path +
jarFile.getAbsolutePath()));
+
assertNull(archive.getCurrentRevision().getContent().getEntryAsNativeLibrary(path
+ jarFile.getAbsolutePath()));
+ assertEquals(0L,
archive.getCurrentRevision().getContent().getContentTime(path +
jarFile.getAbsolutePath()));
}
public void testJarReference() throws Exception
@@ -170,7 +177,7 @@ public class BundleCacheTest extends TestCase
testBundle("bla", jarFile);
}
- private void testBundle(String location, File file) throws Exception
+ private BundleArchive testBundle(String location, File file) throws
Exception
{
BundleArchive archive = cache.create(1, 1, location, file != null ?
new FileInputStream(file) : null, null);
@@ -211,6 +218,8 @@ public class BundleCacheTest extends TestCase
assertFalse(new File(nativeLib).isFile());
assertTrue(new File(nativeLib2).isFile());
assertTrue(new File(nativeLib3).isFile());
+
+ return archive;
}
private void testRevision(BundleArchive archive) throws Exception
