quantranhong1999 commented on code in PR #3197:
URL: https://github.com/apache/james-project/pull/3197#discussion_r4118626741
##########
server/blob/blob-file/src/main/java/org/apache/james/blob/file/FileBlobStoreDAO.java:
##########
@@ -80,25 +77,38 @@ public FileBlobStoreDAO(FileSystem fileSystem,
BlobId.Factory blobIdFactory) thr
@Override
public InputStreamBlob read(BucketName bucketName, BlobId blobId) throws
ObjectStoreIOException, ObjectNotFoundException {
- File bucketRoot = getBucketRoot(bucketName);
- File blob = new File(bucketRoot, blobId.asString());
+ File blob = getBlobFile(bucketName, blobId);
try {
return InputStreamBlob.of(new FileInputStream(blob),
readMetadata(blob.toPath()));
} catch (FileNotFoundException e) {
throw new ObjectNotFoundException(String.format("Cannot locate %s
within %s", blobId.asString(), bucketName.asString()), e);
}
}
+ private File getBlobFile(BucketName bucketName, BlobId blobId) {
+ File bucketRoot = getBucketRoot(bucketName);
+ File blob = new File(bucketRoot, blobId.asString());
+ assertWithinRoot(blob);
Review Comment:
This checks the blob against `root`, not the bucket root, so a blob id like
`../other-bucket/victim` still passes.
Saving `../other-bucket/victim` into `bucket-a` may overwrite `victim` in
`other-bucket`.
If the guard is meant to stop path traversal, it should check
`startsWith(bucketRoot)`, no?
##########
server/blob/blob-file/src/main/java/org/apache/james/blob/file/FileBlobStoreDAO.java:
##########
@@ -80,25 +77,38 @@ public FileBlobStoreDAO(FileSystem fileSystem,
BlobId.Factory blobIdFactory) thr
@Override
public InputStreamBlob read(BucketName bucketName, BlobId blobId) throws
ObjectStoreIOException, ObjectNotFoundException {
- File bucketRoot = getBucketRoot(bucketName);
- File blob = new File(bucketRoot, blobId.asString());
+ File blob = getBlobFile(bucketName, blobId);
try {
return InputStreamBlob.of(new FileInputStream(blob),
readMetadata(blob.toPath()));
} catch (FileNotFoundException e) {
throw new ObjectNotFoundException(String.format("Cannot locate %s
within %s", blobId.asString(), bucketName.asString()), e);
}
}
+ private File getBlobFile(BucketName bucketName, BlobId blobId) {
+ File bucketRoot = getBucketRoot(bucketName);
+ File blob = new File(bucketRoot, blobId.asString());
+ assertWithinRoot(blob);
+ Preconditions.checkArgument(!isStagingFile(blob.toPath()), "Blob name
uses reserved staging prefix: %s", blobId.asString());
+ return blob;
+ }
+
private File getBucketRoot(BucketName bucketName) {
File bucketRoot = new File(root, bucketName.asString());
- if (!bucketRoot.exists()) {
- try {
- FileUtils.forceMkdir(bucketRoot);
- } catch (IOException e) {
- throw new ObjectStoreIOException("Cannot create bucket", e);
+ assertWithinRoot(bucketRoot);
+ return bucketRoot;
+ }
+
+ private void assertWithinRoot(File target) {
+ try {
+ Path canonicalRoot = root.getCanonicalFile().toPath();
+ Path canonicalTarget = target.getCanonicalFile().toPath();
Review Comment:
`getCanonicalFile` resolves symlinks, so a setup where `var/blob/<bucket>`
is a symlink to another disk now fails every read/save/delete with
`IllegalArgumentException`. Master seems to accept that setup;
It also canonicalizes `root` again on each call, which adds 4
canonicalizations per operation on the hot path. Should we canonicalize `root`
once in the constructor and do a lexical `normalize()` +
`startsWith(bucketRoot)` check?
##########
server/blob/blob-file/src/main/java/org/apache/james/blob/file/FileBlobStoreDAO.java:
##########
@@ -174,33 +173,36 @@ private void save(byte[] data, File blob, BlobMetadata
metadata) {
}
private void writeToTempFile(File tempFile, InputStream inputStream,
BlobMetadata metadata) throws IOException {
- try (FileOutputStream out = new FileOutputStream(tempFile);
- FileChannel channel = out.getChannel();
- FileLock fileLock = channel.lock()) {
+ try (FileOutputStream out = new FileOutputStream(tempFile)) {
Review Comment:
This drops the `FileLock` and the `OverlappingFileLockException` retry that
are on master. Since the lock is taken on a unique temp file it's probably a
no-op, but Benoit asked about file locks earlier in this PR and this changes
master behavior after approval. Can you confirm the reasoning in the PR, or
keep it for a separate change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
