diff options
author | Frode Lundgren <frodelu@yahooinc.com> | 2022-04-02 13:06:16 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-04-02 13:06:16 +0200 |
commit | 0c99cb0d778715ea7f3c9d2a14712af35a6738da (patch) | |
tree | 785f3eb12447a2ee063682f37f702af92e3aec6e | |
parent | caa488658d2b5c812e2450f7d91d2a9fb672618f (diff) | |
parent | 7074892f0d55124a9373feae7b18e23855466066 (diff) |
Merge pull request #21950 from vespa-engine/revert-21924-jonmv/misc-7
Revert "Move verification down"
15 files changed, 92 insertions, 89 deletions
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/DistributableResource.java b/config-model/src/main/java/com/yahoo/searchdefinition/DistributableResource.java index e134b8f53ac..245288c283e 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/DistributableResource.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/DistributableResource.java @@ -34,6 +34,11 @@ public class DistributableResource { this.path = path; this.pathType = type; } + public DistributableResource(String name, ByteBuffer blob) { + this.name = name; + path = name + ".lz4"; + pathType = PathType.BLOB; + } //TODO Remove and make path/pathType final public void setFileName(String fileName) { diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/RankExpressionBody.java b/config-model/src/main/java/com/yahoo/searchdefinition/RankExpressionBody.java index 6ba17123fb4..815e4bbf359 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/RankExpressionBody.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/RankExpressionBody.java @@ -6,19 +6,16 @@ import com.yahoo.config.application.api.FileRegistry; import java.nio.ByteBuffer; import java.util.Objects; -import static java.util.Objects.requireNonNull; - public class RankExpressionBody extends DistributableResource { private final ByteBuffer blob; public RankExpressionBody(String name, ByteBuffer blob) { - super(name, name + ".lz4", PathType.BLOB); - this.blob = requireNonNull(blob, "Blob cannot be null"); + super(name, blob); + Objects.requireNonNull(blob, "Blob cannot be null"); + this.blob = blob; } - public ByteBuffer getBlob() { return blob; } - public void validate() { // Remove once pathType is final if (getPathType() != PathType.BLOB) { @@ -29,5 +26,4 @@ public class RankExpressionBody extends DistributableResource { void register(FileRegistry fileRegistry) { register(fileRegistry, blob); } - } diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java b/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java index 0f2c1cf9a15..e7b7c92ca2f 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java @@ -5,7 +5,6 @@ import ai.vespa.rankingexpression.importer.configmodelview.ImportedMlModels; import com.google.common.collect.ImmutableMap; import com.yahoo.config.application.api.ApplicationPackage; import com.yahoo.config.application.api.DeployLogger; -import com.yahoo.path.Path; import com.yahoo.search.query.profile.QueryProfileRegistry; import com.yahoo.search.query.profile.types.FieldDescription; import com.yahoo.search.query.profile.types.QueryProfileType; @@ -883,10 +882,10 @@ public class RankProfile implements Cloneable { if (!expression.startsWith("file:")) return new StringReader(expression); String fileName = extractFileName(expression); - Path.fromString(fileName); // No ".." - if (fileName.contains("/")) // See ticket 4102122 - throw new IllegalArgumentException("In " + name() + ", " + expName + ", ranking references file '" + - fileName + "' in a different directory, which is not supported."); + File file = new File(fileName); + if (!file.isAbsolute() && file.getPath().contains("/")) // See ticket 4102122 + throw new IllegalArgumentException("In " + name() + ", " + expName + ", ranking references file '" + file + + "' in subdirectory, which is not supported."); return schema.getRankingExpression(fileName); } diff --git a/config-model/src/main/javacc/IntermediateParser.jj b/config-model/src/main/javacc/IntermediateParser.jj index 6119eb5e2ce..8a4798d6f74 100644 --- a/config-model/src/main/javacc/IntermediateParser.jj +++ b/config-model/src/main/javacc/IntermediateParser.jj @@ -1773,7 +1773,7 @@ String fileItem() : String path; } { - (<FILE> <COLON> ( <FILE_PATH> | <STRING> | <IDENTIFIER>) { path = com.yahoo.path.Path.fromString(token.image).getRelative(); } { } (<NL>)*) { return path; } + (<FILE> <COLON> ( <FILE_PATH> | <STRING> | <IDENTIFIER>) { path = token.image; } { } (<NL>)*) { return path; } } String uriItem() : { diff --git a/config-model/src/main/javacc/SDParser.jj b/config-model/src/main/javacc/SDParser.jj index c4542c36779..aeffe6e5c39 100644 --- a/config-model/src/main/javacc/SDParser.jj +++ b/config-model/src/main/javacc/SDParser.jj @@ -1907,7 +1907,7 @@ String fileItem() : String path; } { - (<FILE> <COLON> ( <FILE_PATH> | <STRING> | <IDENTIFIER>) { path = com.yahoo.path.Path.fromString(token.image).getRelative(); } { } (<NL>)*) { return path; } + (<FILE> <COLON> ( <FILE_PATH> | <STRING> | <IDENTIFIER>) { path = token.image; } { } (<NL>)*) { return path; } } String uriItem() : { diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/parser/IntermediateParserTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/parser/IntermediateParserTestCase.java index c686a813c9f..36a72381156 100644 --- a/config-model/src/test/java/com/yahoo/searchdefinition/parser/IntermediateParserTestCase.java +++ b/config-model/src/test/java/com/yahoo/searchdefinition/parser/IntermediateParserTestCase.java @@ -62,27 +62,16 @@ public class IntermediateParserTestCase { @Test public void multiple_documents_disallowed() throws Exception { String input = joinLines - ("schema foo {", - " document foo {", - " }", - " document foo2 {", - " }", - "}"); + ("schema foo {", + " document foo {", + " }", + " document foo2 {", + " }", + "}"); var e = assertThrows(IllegalArgumentException.class, () -> parseString(input)); assertEquals("schema 'foo' error: already has document 'foo' so cannot add document 'foo2'", e.getMessage()); } - @Test - public void backwards_path_is_disallowed() { - assertThrows("'..' is not allowed in path", IllegalArgumentException.class, - () -> parseString("schema foo {\n" + - " constant my_constant_tensor {\n" + - " file: foo/../bar\n" + - " type: tensor<float>(x{},y{})\n" + - " }\n" + - "}\n")); - } - void checkFileParses(String fileName) throws Exception { System.err.println("TRY parsing: "+fileName); var schema = parseFile(fileName); diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/AddFileInterface.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/AddFileInterface.java index d51713cff77..855b27e0f92 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/AddFileInterface.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/AddFileInterface.java @@ -2,8 +2,8 @@ package com.yahoo.vespa.config.server.filedistribution; import com.yahoo.config.FileReference; -import com.yahoo.path.Path; +import java.io.File; import java.io.IOException; import java.nio.ByteBuffer; @@ -11,7 +11,8 @@ import java.nio.ByteBuffer; * @author baldersheim */ public interface AddFileInterface { - FileReference addUri(String uri, Path path); - FileReference addFile(Path path) throws IOException; - FileReference addBlob(ByteBuffer blob, Path path); + FileReference addUri(String uri, String relativePath); + FileReference addFile(String relativePath) throws IOException; + FileReference addFile(File file) throws IOException; + FileReference addBlob(ByteBuffer blob, String relativePath); } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/ApplicationFileManager.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/ApplicationFileManager.java index 79fa919fabe..ad47f2b9e95 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/ApplicationFileManager.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/ApplicationFileManager.java @@ -3,10 +3,8 @@ package com.yahoo.vespa.config.server.filedistribution; import com.yahoo.config.FileReference; import com.yahoo.io.IOUtils; -import com.yahoo.path.Path; import net.jpountz.lz4.LZ4FrameOutputStream; -import java.io.Closeable; import java.io.File; import java.io.FileOutputStream; import java.io.IOException; @@ -16,6 +14,8 @@ import java.nio.ByteBuffer; import java.nio.channels.Channels; import java.nio.channels.ReadableByteChannel; import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; /** * @author baldersheim @@ -31,43 +31,53 @@ public class ApplicationFileManager implements AddFileInterface { } @Override - public FileReference addFile(Path path) throws IOException { - File file = new File(applicationDir, path.getRelative()); - return addFile(file); + public FileReference addFile(String relativePath) throws IOException { + Path path = Path.of(relativePath).normalize(); + if (path.isAbsolute()) + throw new IllegalArgumentException(relativePath + " is not relative"); + File file = new File(applicationDir, relativePath); + Path relative = applicationDir.toPath().relativize(file.toPath()).normalize(); + if (relative.isAbsolute() || relative.startsWith("..")) + throw new IllegalArgumentException(file + " is not a descendant of " + applicationDir); + + return fileDirectory.addFile(file); } - private FileReference addFile(File file) throws IOException { + @Override + public FileReference addFile(File file) throws IOException { return fileDirectory.addFile(file); } @Override - public FileReference addUri(String uri, Path path) { - throw new UnsupportedOperationException("URI type is not supported"); - /* TODO: this needs to be super-restricted if the config server should ever do this. - try (TmpDir tmp = new TmpDir()) { - return addFile(download(uri, tmp.dir, path.getRelative())); - } - catch (IOException e) { + public FileReference addUri(String uri, String relativePath) { + File file = download(uri, relativePath); + try { + return addFile(file); + } catch (IOException e) { throw new IllegalArgumentException(e); + } finally { + cleanup(file, relativePath); } - */ } @Override - public FileReference addBlob(ByteBuffer blob, Path path) { - try (TmpDir tmp = new TmpDir()) { - return addFile(writeBlob(blob, tmp.dir, path.getRelative())); - } - catch (IOException e) { + public FileReference addBlob(ByteBuffer blob, String relativePath) { + File file = writeBlob(blob, relativePath); + try { + return addFile(file); + } catch (IOException e) { throw new IllegalArgumentException(e); + } finally { + cleanup(file, relativePath); } } - private File writeBlob(ByteBuffer blob, File tmpDir, String relativePath) { + private File writeBlob(ByteBuffer blob, String relativePath) { FileOutputStream fos = null; File file = null; try { - file = new File(tmpDir, relativePath); + Path path = Files.createTempDirectory(""); + file = new File(path.toFile(), relativePath); Files.createDirectories(file.getParentFile().toPath()); fos = new FileOutputStream(file); if (relativePath.endsWith(".lz4")) { @@ -91,12 +101,13 @@ public class ApplicationFileManager implements AddFileInterface { } } - private File download(String uri, File tmpDir, String relativePath) { + private File download(String uri, String relativePath) { File file = null; FileOutputStream fos = null; ReadableByteChannel rbc = null; try { - file = new File(tmpDir, relativePath); + Path path = Files.createTempDirectory(""); + file = new File(path.toFile(), relativePath); Files.createDirectories(file.getParentFile().toPath()); URL website = new URL(uri); rbc = Channels.newChannel(website.openStream()); @@ -121,10 +132,13 @@ public class ApplicationFileManager implements AddFileInterface { } } - private static class TmpDir implements Closeable { - final File dir = Files.createTempDirectory("").toFile(); - private TmpDir() throws IOException { } - @Override public void close() { IOUtils.recursiveDeleteDir(dir); } + private void cleanup(File file, String relativePath) { + Path pathToDelete = file.toPath(); + // Remove as many components as there are in relative path to find temp path to delete + for (int i = 0; i < Paths.get(relativePath).getNameCount(); i++) + pathToDelete = pathToDelete.resolveSibling(""); + IOUtils.recursiveDeleteDir(pathToDelete.toFile()); } + } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistry.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistry.java index c80a6d1d864..3eeaa6598d3 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistry.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistry.java @@ -5,7 +5,6 @@ import com.google.common.collect.ImmutableMap; import com.yahoo.config.FileReference; import com.yahoo.config.application.api.FileRegistry; import com.yahoo.net.HostName; -import com.yahoo.path.Path; import com.yahoo.text.Utf8; import net.jpountz.xxhash.XXHashFactory; @@ -71,13 +70,10 @@ public class FileDBRegistry implements FileRegistry { @Override public synchronized FileReference addFile(String relativePath) { - if (relativePath.startsWith("/")) - throw new IllegalArgumentException(relativePath + " is not relative"); - Optional<FileReference> cachedReference = Optional.ofNullable(fileReferenceCache.get(relativePath)); return cachedReference.orElseGet(() -> { try { - FileReference newRef = manager.addFile(Path.fromString(relativePath)); + FileReference newRef = manager.addFile(relativePath); fileReferenceCache.put(relativePath, newRef); return newRef; } catch (FileNotFoundException e) { @@ -98,7 +94,7 @@ public class FileDBRegistry implements FileRegistry { String relativePath = uriToRelativeFile(uri); Optional<FileReference> cachedReference = Optional.ofNullable(fileReferenceCache.get(uri)); return cachedReference.orElseGet(() -> { - FileReference newRef = manager.addUri(uri, Path.fromString(relativePath)); + FileReference newRef = manager.addUri(uri, relativePath); fileReferenceCache.put(uri, newRef); return newRef; }); @@ -110,7 +106,7 @@ public class FileDBRegistry implements FileRegistry { synchronized (this) { Optional<FileReference> cachedReference = Optional.ofNullable(fileReferenceCache.get(blobName)); return cachedReference.orElseGet(() -> { - FileReference newRef = manager.addBlob(blob, Path.fromString(relativePath)); + FileReference newRef = manager.addBlob(blob, relativePath); fileReferenceCache.put(blobName, newRef); return newRef; }); diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDirectory.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDirectory.java index 46ae2fd15d5..f4cb7bc1fba 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDirectory.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDirectory.java @@ -135,7 +135,7 @@ public class FileDirectory { return new FileReference(Long.toHexString(hash)); } - private FileReference addFile(File source, FileReference reference) { + FileReference addFile(File source, FileReference reference) { ensureRootExist(); try { logfileInfo(source); diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/MockFileManager.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/MockFileManager.java index 6f551e40716..ce0a9866919 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/MockFileManager.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/MockFileManager.java @@ -2,7 +2,6 @@ package com.yahoo.vespa.config.server.filedistribution; import com.yahoo.config.FileReference; -import com.yahoo.path.Path; import java.io.File; import java.nio.ByteBuffer; @@ -13,17 +12,22 @@ import java.nio.ByteBuffer; public class MockFileManager implements AddFileInterface { @Override - public FileReference addUri(String uri, Path path) { + public FileReference addUri(String uri, String relativePath) { return null; } @Override - public FileReference addFile(Path path) { + public FileReference addFile(String relativePath) { return null; } @Override - public FileReference addBlob(ByteBuffer blob, Path path) { + public FileReference addFile(File file) { + return null; + } + + @Override + public FileReference addBlob(ByteBuffer blob, String relativePath) { return null; } diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistryTestCase.java b/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistryTestCase.java index 6d7074aef3c..cdb01f2013b 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistryTestCase.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/FileDBRegistryTestCase.java @@ -27,7 +27,7 @@ public class FileDBRegistryTestCase { private static final String NO_FOO_FILE = "files/no_foo.json"; private static final String BOO_FILE = "/files/no_foo.json"; private static final String BAR_FILE = "../files/no_foo.json"; - private static final String BLOB_NAME = "././myblob.name"; + private static final String BLOB_NAME = "myblob.name"; private static final FileReference BLOB_REF = new FileReference("12f292a25163dd9"); private static final FileReference FOO_REF = new FileReference("b5ce94ca1feae86c"); @@ -54,7 +54,7 @@ public class FileDBRegistryTestCase { fileRegistry.addFile(BAR_FILE); fail(); } catch (IllegalArgumentException e) { - assertEquals("'..' is not allowed in path", e.getMessage()); + assertEquals("src/test/apps/zkapp/../files/no_foo.json is not a descendant of src/test/apps/zkapp", e.getMessage()); } assertEquals(BLOB_REF, fileRegistry.addBlob(BLOB_NAME, ByteBuffer.wrap(BLOB.getBytes(StandardCharsets.UTF_8)))); String serializedRegistry = FileDBRegistry.exportRegistry(fileRegistry); @@ -78,5 +78,4 @@ public class FileDBRegistryTestCase { void checkConsistentEntry(FileRegistry.Entry entry, FileRegistry registry) { assertEquals(entry.reference, registry.addFile(entry.relativePath)); } - } diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/MockFileDistributionFactory.java b/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/MockFileDistributionFactory.java index 6c65a2dee81..3ac621f7ac4 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/MockFileDistributionFactory.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/MockFileDistributionFactory.java @@ -17,7 +17,7 @@ public class MockFileDistributionFactory extends FileDistributionFactory { @Override public FileRegistry createFileRegistry(File applicationPackage) { - return new MockFileRegistry(applicationPackage, getFileReferencesDir()); + return new MockFileRegistry(applicationPackage, getFileReferencesDir().toPath()); } @Override diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/MockFileRegistry.java b/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/MockFileRegistry.java index 9a9c224627a..be2d087478d 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/MockFileRegistry.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/filedistribution/MockFileRegistry.java @@ -3,11 +3,11 @@ package com.yahoo.vespa.config.server.filedistribution; import com.yahoo.config.FileReference; import com.yahoo.config.application.api.FileRegistry; -import com.yahoo.path.Path; import java.io.File; import java.io.IOException; import java.nio.ByteBuffer; +import java.nio.file.Path; import java.util.ArrayList; import java.util.List; @@ -21,15 +21,18 @@ public class MockFileRegistry implements FileRegistry { private final List<Entry> entries = new ArrayList<>(); private final AddFileInterface addFileInterface; - public MockFileRegistry(File applicationDir, File rootPath) { - FileDirectory fileDirectory = new FileDirectory(rootPath); + public MockFileRegistry(File applicationDir, Path rootPath) { + FileDirectory fileDirectory = new FileDirectory(rootPath.toFile()); this.addFileInterface = new ApplicationFileManager(applicationDir, fileDirectory); } public FileReference addFile(String relativePath) { - if (relativePath.isEmpty()) relativePath = "./"; + if (relativePath.isEmpty()) + relativePath = "./"; + try { - addFileInterface.addFile(Path.fromString(relativePath)); + addFileInterface.addFile(relativePath); + FileReference fileReference = new FileReference(relativePath); entries.add(new Entry(relativePath, fileReference)); return fileReference; @@ -47,10 +50,10 @@ public class MockFileRegistry implements FileRegistry { @Override public FileReference addBlob(String name, ByteBuffer blob) { - name = "./" + name; - FileReference fileReference = addFileInterface.addBlob(blob, Path.fromString(name)); + String relativePath = "./" + name; + FileReference fileReference = addFileInterface.addBlob(blob, relativePath); - entries.add(new Entry(name, fileReference)); + entries.add(new Entry(relativePath, fileReference)); return fileReference; } diff --git a/vespajlib/src/test/java/ai/vespa/validation/NameTest.java b/vespajlib/src/test/java/ai/vespa/validation/NameTest.java index 0b50340870e..e6645ee4fc3 100644 --- a/vespajlib/src/test/java/ai/vespa/validation/NameTest.java +++ b/vespajlib/src/test/java/ai/vespa/validation/NameTest.java @@ -4,9 +4,6 @@ package ai.vespa.validation; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; -import java.nio.file.Path; - -import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; /** |