diff --git a/CHANGES.md b/CHANGES.md index 107c422ef..7f57f6aa1 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -29,6 +29,7 @@ * [GH-370](https://github.com/apache/mina-sshd/issues/370) Also compare file keys in `ModifiableFileWatcher`. * [GH-371](https://github.com/apache/mina-sshd/issues/371) Fix channel pool in `SftpFileSystem`. * [GH-383](https://github.com/apache/mina-sshd/issues/383) Use correct default `OpenOption`s in `SftpFileSystemProvider.newFileChannel()`. +* [GH-384](https://github.com/apache/mina-sshd/issues/384) Use correct lock modes for SFTP `FileChannel.lock()`. * [SSHD-1259](https://issues.apache.org/jira/browse/SSHD-1259) Consider all applicable host keys from the known_hosts files. * [SSHD-1310](https://issues.apache.org/jira/browse/SSHD-1310) `SftpFileSystem`: do not close user session. diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/AbstractSftpClient.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/AbstractSftpClient.java index 8a8bcd941..2c3b8ea7f 100644 --- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/AbstractSftpClient.java +++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/AbstractSftpClient.java @@ -1091,7 +1091,7 @@ public void link(String linkPath, String targetPath, boolean symbolic) throws IO Buffer buffer = new ByteArrayBuffer(linkPath.length() + targetPath.length() + Long.SIZE /* some extra fields */, false); if (version < SftpConstants.SFTP_V6) { if (!symbolic) { - throw new UnsupportedOperationException("Hard links are not supported in sftp v" + version); + throw new UnsupportedOperationException("Hard links are not supported in sftp v" + version + ", need SFTPv6"); } buffer = putReferencedName(cmd, buffer, targetPath, 0); buffer = putReferencedName(cmd, buffer, linkPath, 1); @@ -1110,7 +1110,10 @@ public void lock(Handle handle, long offset, long length, int mask) throws IOExc "lock(" + handle + ")[offset=" + offset + ", length=" + length + ", mask=0x" + Integer.toHexString(mask) + "] client is closed"); } - + int version = getVersion(); + if (version < SftpConstants.SFTP_V6) { + throw new UnsupportedOperationException("File locks are not supported in sftp v" + version + ", need SFTPv6"); + } byte[] id = Objects.requireNonNull(handle, "No handle").getIdentifier(); Buffer buffer = new ByteArrayBuffer(id.length + Long.SIZE /* a bit extra */, false); buffer.putBytes(id); @@ -1129,6 +1132,10 @@ public void unlock(Handle handle, long offset, long length) throws IOException { if (!isOpen()) { throw new IOException("unlock" + handle + ")[offset=" + offset + ", length=" + length + "] client is closed"); } + int version = getVersion(); + if (version < SftpConstants.SFTP_V6) { + throw new UnsupportedOperationException("File locks are not supported in sftp v" + version + ", need SFTPv6"); + } byte[] id = Objects.requireNonNull(handle, "No handle").getIdentifier(); Buffer buffer = new ByteArrayBuffer(id.length + Long.SIZE /* a bit extra */, false); diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpRemotePathChannel.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpRemotePathChannel.java index 62699c34b..1fcc1f53f 100644 --- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpRemotePathChannel.java +++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpRemotePathChannel.java @@ -27,6 +27,8 @@ import java.nio.channels.ClosedChannelException; import java.nio.channels.FileChannel; import java.nio.channels.FileLock; +import java.nio.channels.NonReadableChannelException; +import java.nio.channels.NonWritableChannelException; import java.nio.channels.OverlappingFileLockException; import java.nio.channels.ReadableByteChannel; import java.nio.channels.WritableByteChannel; @@ -41,7 +43,6 @@ import java.util.concurrent.atomic.AtomicReference; import org.apache.sshd.client.session.ClientSession; -import org.apache.sshd.common.util.GenericUtils; import org.apache.sshd.common.util.ValidateUtils; import org.apache.sshd.sftp.SftpModuleProperties; import org.apache.sshd.sftp.client.SftpClient; @@ -78,10 +79,13 @@ public SftpRemotePathChannel(String path, SftpClient sftp, boolean closeOnExit, throws IOException { this.log = LoggerFactory.getLogger(getClass()); this.path = ValidateUtils.checkNotNullAndNotEmpty(path, "No remote file path specified"); - this.modes = Objects.requireNonNull(modes, "No channel modes specified"); + this.modes = Collections.unmodifiableSet(EnumSet.copyOf(modes)); + if (this.modes.isEmpty()) { + throw new IllegalArgumentException("At least one OpenMode is required for a SftpRemotePathChannel"); + } this.sftp = Objects.requireNonNull(sftp, "No SFTP client instance"); this.closeOnExit = closeOnExit; - this.handle = sftp.open(path, modes); + this.handle = sftp.open(path, this.modes); } public String getRemotePath() { @@ -119,7 +123,10 @@ public long read(ByteBuffer[] dsts, int offset, int length) throws IOException { } protected long doRead(Collection buffers, long position) throws IOException { - ensureOpen(READ_MODES); + if (!isOpen()) { + throw new ClosedChannelException(); + } + ensureMode(false); ClientSession clientSession = sftp.getClientSession(); int copySize = SftpModuleProperties.COPY_BUF_SIZE.getRequired(clientSession); @@ -223,7 +230,10 @@ public long write(ByteBuffer[] srcs, int offset, int length) throws IOException } protected long doWrite(Collection buffers, long position) throws IOException { - ensureOpen(WRITE_MODES); + if (!isOpen()) { + throw new ClosedChannelException(); + } + ensureMode(true); ClientSession clientSession = sftp.getClientSession(); int copySize = SftpModuleProperties.COPY_BUF_SIZE.getRequired(clientSession); @@ -282,7 +292,9 @@ protected long doWrite(Collection buffers, long position) @Override public long position() throws IOException { - ensureOpen(Collections.emptySet()); + if (!isOpen()) { + throw new ClosedChannelException(); + } return posTracker.get(); } @@ -293,28 +305,37 @@ public FileChannel position(long newPosition) throws IOException { + " illegal file channel position: " + newPosition); } - ensureOpen(Collections.emptySet()); + if (!isOpen()) { + throw new ClosedChannelException(); + } posTracker.set(newPosition); return this; } @Override public long size() throws IOException { - ensureOpen(Collections.emptySet()); + if (!isOpen()) { + throw new ClosedChannelException(); + } Attributes stat = sftp.stat(handle); return stat.getSize(); } @Override public FileChannel truncate(long size) throws IOException { - ensureOpen(Collections.emptySet()); + if (!isOpen()) { + throw new ClosedChannelException(); + } + ensureMode(true); sftp.setStat(handle, new SftpClient.Attributes().size(size)); return this; } @Override public void force(boolean metaData) throws IOException { - ensureOpen(Collections.emptySet()); + if (!isOpen()) { + throw new ClosedChannelException(); + } } @Override @@ -323,7 +344,10 @@ public long transferTo(long position, long count, WritableByteChannel target) th throw new IllegalArgumentException("transferTo(" + getRemotePath() + ")" + " illegal position (" + position + ") or count (" + count + ")"); } - ensureOpen(READ_MODES); + if (!isOpen() || !target.isOpen()) { + throw new ClosedChannelException(); + } + ensureMode(false); ClientSession clientSession = sftp.getClientSession(); int copySize = SftpModuleProperties.COPY_BUF_SIZE.getRequired(clientSession); @@ -359,7 +383,10 @@ public long transferTo(long position, long count, WritableByteChannel target) th this, position, count, copySize, totalRead, eof, target); } - return (totalRead > 0L) ? totalRead : eof ? -1L : 0L; + if (totalRead > 0) { + return totalRead; + } + return eof ? -1 : 0; } @Override @@ -368,7 +395,10 @@ public long transferFrom(ReadableByteChannel src, long position, long count) thr throw new IllegalArgumentException("transferFrom(" + getRemotePath() + ")" + " illegal position (" + position + ") or count (" + count + ")"); } - ensureOpen(WRITE_MODES); + if (!isOpen() || !src.isOpen()) { + throw new ClosedChannelException(); + } + ensureMode(true); ClientSession clientSession = sftp.getClientSession(); int copySize = SftpModuleProperties.COPY_BUF_SIZE.getRequired(clientSession); @@ -431,10 +461,14 @@ public FileLock lock(long position, long size, boolean shared) throws IOExceptio @Override public FileLock tryLock(long position, long size, boolean shared) throws IOException { - ensureOpen(Collections.emptySet()); + if (!isOpen()) { + throw new ClosedChannelException(); + } + ensureMode(!shared); + int lockFlags = shared ? SftpConstants.SSH_FXF_READ_LOCK : SftpConstants.SSH_FXF_WRITE_LOCK; try { - sftp.lock(handle, position, size, 0); + sftp.lock(handle, position, size, lockFlags); } catch (SftpException e) { if (e.getStatus() == SftpConstants.SSH_FX_LOCK_CONFLICT) { throw new OverlappingFileLockException(); @@ -452,7 +486,7 @@ public boolean isValid() { @Override public void release() throws IOException { - if (valid.compareAndSet(true, false)) { + if (valid.getAndSet(false)) { sftp.unlock(handle, position, size); } } @@ -500,27 +534,15 @@ protected void endBlocking(Object actionHint, boolean completed) end(completed); } - /** - * Checks that the channel is open and that its current mode contains at least one of the required ones - * - * @param reqModes The required modes - ignored if {@code null}/empty - * @throws IOException If channel not open or the required modes are not satisfied - */ - private void ensureOpen(Collection reqModes) throws IOException { - if (!isOpen()) { - throw new ClosedChannelException(); - } - - if (GenericUtils.size(reqModes) > 0) { - for (OpenMode m : reqModes) { - if (this.modes.contains(m)) { - return; - } + private void ensureMode(boolean forWriting) { + if (!forWriting && !modes.contains(OpenMode.Read)) { + throw new NonReadableChannelException(); + } else if (forWriting) { + EnumSet myModes = EnumSet.copyOf(modes); + myModes.retainAll(WRITE_MODES); + if (myModes.isEmpty()) { + throw new NonWritableChannelException(); } - - throw new IOException("ensureOpen(" + getRemotePath() + ")" - + " current channel modes (" + this.modes + ")" - + " do contain any of the required ones: " + reqModes); } } diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/FileHandle.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/FileHandle.java index af98a8dbc..88dcac8e5 100644 --- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/FileHandle.java +++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/FileHandle.java @@ -21,6 +21,8 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.nio.channels.FileLock; +import java.nio.channels.NonReadableChannelException; +import java.nio.channels.NonWritableChannelException; import java.nio.channels.SeekableByteChannel; import java.nio.file.Path; import java.nio.file.StandardOpenOption; @@ -164,22 +166,70 @@ public void close() throws IOException { } public void lock(long offset, long length, int mask) throws IOException { + // We map delete locks to write locks, and we ignore the advisory bit. + boolean writeLock = (mask & (SftpConstants.SSH_FXF_WRITE_LOCK | SftpConstants.SSH_FXF_DELETE_LOCK)) != 0; + if (!writeLock) { + // If read and write are requested, it's a write lock. + boolean readLock = (mask & SftpConstants.SSH_FXF_READ_LOCK) != 0; + // Draft RFC is silent on what to do if no flags at all are set: + // https://www.ietf.org/archive/id/draft-ietf-secsh-filexfer-13.txt + // If the handle was opened for reading, use a read lock, otherwise a write lock. + if (!readLock && canWrite()) { + writeLock = true; + } + } + if (writeLock && !canWrite()) { + throw new SftpException(SftpConstants.SSH_FX_BYTE_RANGE_LOCK_REFUSED, + "Write lock requested, but handle opened for reading only"); + } else if (!writeLock && !canRead()) { + throw new SftpException(SftpConstants.SSH_FX_BYTE_RANGE_LOCK_REFUSED, + "Read lock requested, but handle opened for writing only"); + } SeekableByteChannel channel = getFileChannel(); long size = (length == 0L) ? channel.size() - offset : length; SftpSubsystem subsystem = getSubsystem(); SftpFileSystemAccessor accessor = subsystem.getFileSystemAccessor(); - FileLock lock = accessor.tryLock( - subsystem, this, getFile(), getFileHandle(), channel, offset, size, false); - if (lock == null) { - throw new SftpException(SftpConstants.SSH_FX_BYTE_RANGE_LOCK_REFUSED, - "Overlapping lock held by another program on range [" + offset + "-" + (offset + length)); + FileLock lock = null; + try { + lock = accessor.tryLock(subsystem, this, getFile(), getFileHandle(), channel, offset, size, !writeLock); + } catch (NonReadableChannelException | NonWritableChannelException e) { + SftpException error = new SftpException(SftpConstants.SSH_FX_BYTE_RANGE_LOCK_REFUSED, + "Could not acquire channel lock; write=" + writeLock + ": " + e.toString()); + error.initCause(e); + throw error; } + if (lock == null) { + throw new SftpException(SftpConstants.SSH_FX_BYTE_RANGE_LOCK_CONFLICT, + "Overlapping lock held by another program on range [" + offset + "-" + (offset + length) + "]"); + } synchronized (locks) { locks.add(lock); } } + private boolean canRead() { + return getOpenOptions().contains(StandardOpenOption.READ); + } + + private boolean canWrite() { + for (StandardOpenOption option : getOpenOptions()) { + switch (option) { + case APPEND: + case CREATE: + case CREATE_NEW: + case DELETE_ON_CLOSE: + case TRUNCATE_EXISTING: + case WRITE: + return true; + default: + break; + + } + } + return false; + } + public void unlock(long offset, long length) throws IOException { SeekableByteChannel channel = getFileChannel(); long size = (length == 0L) ? channel.size() - offset : length; diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpSubsystem.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpSubsystem.java index 8011cc810..868f22edd 100644 --- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpSubsystem.java +++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/server/SftpSubsystem.java @@ -534,6 +534,10 @@ protected void doVersionSelect(Buffer buffer, int id, String proposed) throws IO @Override protected void doBlock(int id, String handle, long offset, long length, int mask) throws IOException { + int vers = getVersion(); + if (vers < SftpConstants.SFTP_V6) { + throw new UnsupportedOperationException("File locks are not supported in sftp v" + vers + ", need SFTPv6"); + } Handle p = handles.get(handle); ServerSession session = getServerSession(); if (log.isDebugEnabled()) { @@ -555,6 +559,10 @@ protected void doBlock(int id, String handle, long offset, long length, int mask @Override protected void doUnblock(int id, String handle, long offset, long length) throws IOException { + int vers = getVersion(); + if (vers < SftpConstants.SFTP_V6) { + throw new UnsupportedOperationException("File locks are not supported in sftp v" + vers + ", need SFTPv6"); + } Handle p = handles.get(handle); ServerSession session = getServerSession(); if (log.isDebugEnabled()) { diff --git a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/fs/AbstractSftpFilesSystemSupport.java b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/fs/AbstractSftpFilesSystemSupport.java index 35969830c..5750d1177 100644 --- a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/fs/AbstractSftpFilesSystemSupport.java +++ b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/fs/AbstractSftpFilesSystemSupport.java @@ -25,6 +25,8 @@ import java.nio.ByteBuffer; import java.nio.channels.FileChannel; import java.nio.channels.FileLock; +import java.nio.channels.NonReadableChannelException; +import java.nio.channels.NonWritableChannelException; import java.nio.channels.OverlappingFileLockException; import java.nio.charset.StandardCharsets; import java.nio.file.DirectoryStream; @@ -160,7 +162,11 @@ protected void testFileSystem(FileSystem fs, int version) throws Exception { outputDebugMessage("%s no-follow attributes: %s", file1, attrs); assertEquals("Mismatched symlink data", expected, new String(Files.readAllBytes(file1), StandardCharsets.UTF_8)); - testFileChannelLock(file1); + if (version == SftpConstants.SFTP_V6) { + testFileChannelLock(file1); + } else { + assertThrows(UnsupportedOperationException.class, () -> testFileChannelLock(file1)); + } Files.delete(file1); } @@ -240,6 +246,17 @@ protected static void testSymbolicLinks(Path link, Path relPath) throws IOExcept } protected static void testFileChannelLock(Path file) throws IOException { + testFileChannelLockOverlap(file); + testFileChannelLockWriteRead(file); + testFileChannelLockWriteWrite(file); + testFileChannelLockAppendRead(file); + testFileChannelLockAppendWrite(file); + testFileChannelLockReadWrite(file); + testFileChannelLockReadRead(file); + testFileChannelLockBoth(file); + } + + protected static void testFileChannelLockOverlap(Path file) throws IOException { try (FileChannel channel = FileChannel.open(file, StandardOpenOption.WRITE)) { try (FileLock lock = channel.lock()) { outputDebugMessage("Lock %s: %s", file, lock); @@ -255,6 +272,64 @@ protected static void testFileChannelLock(Path file) throws IOException { } } + protected static void testFileChannelLockWriteRead(Path file) throws IOException { + try (FileChannel channel = FileChannel.open(file, StandardOpenOption.WRITE)) { + assertThrows(NonReadableChannelException.class, () -> channel.lock(0, Long.MAX_VALUE, true)); + } + } + + protected static void testFileChannelLockWriteWrite(Path file) throws IOException { + try (FileChannel channel = FileChannel.open(file, StandardOpenOption.WRITE)) { + try (FileLock lock = channel.lock()) { + outputDebugMessage("Lock %s: %s", file, lock); + } + } + } + + protected static void testFileChannelLockAppendRead(Path file) throws IOException { + try (FileChannel channel = FileChannel.open(file, StandardOpenOption.APPEND)) { + assertThrows(NonReadableChannelException.class, () -> channel.lock(0, Long.MAX_VALUE, true)); + } + } + + protected static void testFileChannelLockAppendWrite(Path file) throws IOException { + try (FileChannel channel = FileChannel.open(file, StandardOpenOption.APPEND)) { + try (FileLock lock = channel.lock()) { + outputDebugMessage("Lock %s: %s", file, lock); + } + } + } + + protected static void testFileChannelLockReadWrite(Path file) throws IOException { + try (FileChannel channel = FileChannel.open(file)) { + assertThrows(NonWritableChannelException.class, () -> channel.lock(0, Long.MAX_VALUE, false)); + } + } + + protected static void testFileChannelLockReadRead(Path file) throws IOException { + try (FileChannel channel = FileChannel.open(file, StandardOpenOption.READ)) { + try (FileLock lock = channel.lock(0, Long.MAX_VALUE, true)) { + outputDebugMessage("Lock %s: %s", file, lock); + } + } + try (FileChannel channel = FileChannel.open(file)) { // READ is default + try (FileLock lock = channel.lock(0, Long.MAX_VALUE, true)) { + outputDebugMessage("Lock %s: %s", file, lock); + } + } + } + + protected static void testFileChannelLockBoth(Path file) throws IOException { + try (FileChannel channel = FileChannel.open(file, StandardOpenOption.READ, StandardOpenOption.WRITE)) { + try (FileLock lock = channel.lock(0, Long.MAX_VALUE, false)) { + outputDebugMessage("Lock %s: %s", file, lock); + } + try (FileLock lock = channel.lock(0, Long.MAX_VALUE, true)) { + outputDebugMessage("Lock %s: %s", file, lock); + } + } + } + protected static FileSystem createSftpFileSystem(ClientSession session, SftpVersionSelector selector) throws IOException { return SftpClientFactory.instance().createSftpFileSystem(session, selector); }