mirrors/CC-Tweaked
1
0
Fork 0
mirror of https://github.com/SquidDev-CC/CC-Tweaked synced 2024-12-13 03:30:29 +00:00
Code Issues Releases Wiki Activity

Fix mounts being usable after a disk is ejected

Browse Source 
This probably fails "responsible disclosure", but it's not an RCE and
frankly the whole bug is utterly hilarious so here we are...

It's possible to open a file on a disk drive and continue to read/write
to them after the disk has been removed:

    local disk = peripheral.find("drive")
    local input = fs.open(fs.combine(disk.getMountPath(), "stream"), "rb")
    local output = fs.open(fs.combine(disk.getMountPath(), "stream"), "wb")
    disk.ejectDisk()

    -- input/output can still be interacted with.

This is pretty amusing, as now it allows us to move the disk somewhere
else and repeat - we've now got a private tunnel which two computers can
use to communicate.

Fixing this is intuitively quite simple - just close any open files
belonging to this mount. However, this is where things get messy thanks
to the wonderful joy of how CC's streams are handled.

As things stand, the filesystem effectively does the following flow::
 - There is a function `open : String -> Channel' (file modes are
   irrelevant here).

 - Once a file is opened, we transform it into some <T extends
   Closeable>. This is, for instance, a BufferedReader.

 - We generate a "token" (i.e. FileSystemWrapper<T>), which we generate
   a week reference to and map it to a tuple of our Channel and T. If
   this token is ever garbage collected (someone forgot to call close()
   on a file), then we close our T and Channel.

 - This token and T are returned to the calling function, which then
   constructs a Lua object.

The problem here is that if we close the underlying Channel+T before the
Lua object calls .close(), then it won't know the underlying channel is
closed, and you get some pretty ugly errors (e.g. "Stream Closed"). So
we've moved the "is open" state into the FileSystemWrapper<T>.

The whole system is incredibly complex at this point, and I'd really
like to clean it up. Ideally we could treat the HandleGeneric as the
token instead - this way we could potentially also clean up
FileSystemWrapperMount.

BBut something to play with in the future, and not when it's 10:30pm.

---

All this wall of text, and this isn't the only bug I've found with disks
today :/.
This commit is contained in:
Jonathan Coates 2021-01-13 22:10:44 +00:00
parent 1f84480a80
commit 1255bd00fd
10 changed files with 137 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
src/main/java/dan200/computercraft/core/apis/handles/BinaryReadableHandle.java
View File

@ -7,6 +7,7 @@ package dan200.computercraft.core.apis.handles;
import dan200.computercraft.api.lua.LuaException;
import dan200.computercraft.api.lua.LuaFunction;
import dan200.computercraft.core.filesystem.TrackingCloseable;
import java.io.ByteArrayOutputStream;
import java.io.Closeable;
@ -32,14 +33,14 @@ public class BinaryReadableHandle extends HandleGeneric
final SeekableByteChannel seekable;
private final ByteBuffer single = ByteBuffer.allocate( 1 );
BinaryReadableHandle( ReadableByteChannel reader, SeekableByteChannel seekable, Closeable closeable )
BinaryReadableHandle( ReadableByteChannel reader, SeekableByteChannel seekable, TrackingCloseable closeable )
{
super( closeable );
this.reader = reader;
this.seekable = seekable;
}
public static BinaryReadableHandle of( ReadableByteChannel channel, Closeable closeable )
public static BinaryReadableHandle of( ReadableByteChannel channel, TrackingCloseable closeable )
{
SeekableByteChannel seekable = asSeekable( channel );
return seekable == null ? new BinaryReadableHandle( channel, null, closeable ) : new Seekable( seekable, closeable );
@ -47,7 +48,7 @@ public class BinaryReadableHandle extends HandleGeneric
public static BinaryReadableHandle of( ReadableByteChannel channel )
{
return of( channel, channel );
return of( channel, new TrackingCloseable.Impl( channel ) );
}
/**
@ -237,7 +238,7 @@ public class BinaryReadableHandle extends HandleGeneric
public static class Seekable extends BinaryReadableHandle
{
Seekable( SeekableByteChannel seekable, Closeable closeable )
Seekable( SeekableByteChannel seekable, TrackingCloseable closeable )
{
super( seekable, seekable, closeable );
}

9
src/main/java/dan200/computercraft/core/apis/handles/BinaryWritableHandle.java
View File

@ -9,6 +9,7 @@ import dan200.computercraft.api.lua.IArguments;
import dan200.computercraft.api.lua.LuaException;
import dan200.computercraft.api.lua.LuaFunction;
import dan200.computercraft.api.lua.LuaValues;
import dan200.computercraft.core.filesystem.TrackingCloseable;
import java.io.Closeable;
import java.io.IOException;
@ -30,14 +31,14 @@ public class BinaryWritableHandle extends HandleGeneric
final SeekableByteChannel seekable;
private final ByteBuffer single = ByteBuffer.allocate( 1 );
protected BinaryWritableHandle( WritableByteChannel writer, SeekableByteChannel seekable, Closeable closeable )
protected BinaryWritableHandle( WritableByteChannel writer, SeekableByteChannel seekable, TrackingCloseable closeable )
{
super( closeable );
this.writer = writer;
this.seekable = seekable;
}
public static BinaryWritableHandle of( WritableByteChannel channel, Closeable closeable )
public static BinaryWritableHandle of( WritableByteChannel channel, TrackingCloseable closeable )
{
SeekableByteChannel seekable = asSeekable( channel );
return seekable == null ? new BinaryWritableHandle( channel, null, closeable ) : new Seekable( seekable, closeable );
@ -45,7 +46,7 @@ public class BinaryWritableHandle extends HandleGeneric
public static BinaryWritableHandle of( WritableByteChannel channel )
{
return of( channel, channel );
return of( channel, new TrackingCloseable.Impl( channel ) );
}
/**
@ -108,7 +109,7 @@ public class BinaryWritableHandle extends HandleGeneric
public static class Seekable extends BinaryWritableHandle
{
public Seekable( SeekableByteChannel seekable, Closeable closeable )
public Seekable( SeekableByteChannel seekable, TrackingCloseable closeable )
{
super( seekable, seekable, closeable );
}

5
src/main/java/dan200/computercraft/core/apis/handles/EncodedReadableHandle.java
View File

@ -7,6 +7,7 @@ package dan200.computercraft.core.apis.handles;
import dan200.computercraft.api.lua.LuaException;
import dan200.computercraft.api.lua.LuaFunction;
import dan200.computercraft.core.filesystem.TrackingCloseable;
import javax.annotation.Nonnull;
import java.io.BufferedReader;
@ -32,7 +33,7 @@ public class EncodedReadableHandle extends HandleGeneric
private final BufferedReader reader;
public EncodedReadableHandle( @Nonnull BufferedReader reader, @Nonnull Closeable closable )
public EncodedReadableHandle( @Nonnull BufferedReader reader, @Nonnull TrackingCloseable closable )
{
super( closable );
this.reader = reader;
@ -40,7 +41,7 @@ public class EncodedReadableHandle extends HandleGeneric
public EncodedReadableHandle( @Nonnull BufferedReader reader )
{
this( reader, reader );
this( reader, new TrackingCloseable.Impl( reader ) );
}
/**

3
src/main/java/dan200/computercraft/core/apis/handles/EncodedWritableHandle.java
View File

@ -8,6 +8,7 @@ package dan200.computercraft.core.apis.handles;
import dan200.computercraft.api.lua.IArguments;
import dan200.computercraft.api.lua.LuaException;
import dan200.computercraft.api.lua.LuaFunction;
import dan200.computercraft.core.filesystem.TrackingCloseable;
import dan200.computercraft.shared.util.StringUtil;
import javax.annotation.Nonnull;
@ -30,7 +31,7 @@ public class EncodedWritableHandle extends HandleGeneric
{
private final BufferedWriter writer;
public EncodedWritableHandle( @Nonnull BufferedWriter writer, @Nonnull Closeable closable )
public EncodedWritableHandle( @Nonnull BufferedWriter writer, @Nonnull TrackingCloseable closable )
{
super( closable );
this.writer = writer;

18
src/main/java/dan200/computercraft/core/apis/handles/HandleGeneric.java
View File

@ -7,10 +7,10 @@ package dan200.computercraft.core.apis.handles;
import dan200.computercraft.api.lua.LuaException;
import dan200.computercraft.api.lua.LuaFunction;
import dan200.computercraft.core.filesystem.TrackingCloseable;
import dan200.computercraft.shared.util.IoUtil;
import javax.annotation.Nonnull;
import java.io.Closeable;
import java.io.IOException;
import java.nio.channels.Channel;
import java.nio.channels.SeekableByteChannel;
@ -18,25 +18,23 @@ import java.util.Optional;
public abstract class HandleGeneric
{
private Closeable closable;
private boolean open = true;
private TrackingCloseable closeable;
protected HandleGeneric( @Nonnull Closeable closable )
protected HandleGeneric( @Nonnull TrackingCloseable closeable )
{
this.closable = closable;
this.closeable = closeable;
}
protected void checkOpen() throws LuaException
{
if( !open ) throw new LuaException( "attempt to use a closed file" );
TrackingCloseable closeable = this.closeable;
if( closeable == null || !closeable.isOpen() ) throw new LuaException( "attempt to use a closed file" );
}
protected final void close()
{
open = false;
IoUtil.closeQuietly( closable );
closable = null;
IoUtil.closeQuietly( closeable );
closeable = null;
}
/**

2
src/main/java/dan200/computercraft/core/filesystem/ChannelWrapper.java
View File

@ -42,7 +42,7 @@ class ChannelWrapper<T extends Closeable> implements Closeable
}
}
public T get()
T get()
{
return wrapper;
}

42
src/main/java/dan200/computercraft/core/filesystem/FileSystem.java
View File

@ -95,7 +95,29 @@ public class FileSystem
public synchronized void unmount( String path )
{
mounts.remove( sanitizePath( path ) );
MountWrapper mount = mounts.remove( sanitizePath( path ) );
if( mount == null ) return;
cleanup();
// Close any files which belong to this mount - don't want people writing to a disk after it's been ejected!
// There's no point storing a Mount -> Wrapper[] map, as m_openFiles is small and unmount isn't called very
// often.
synchronized( m_openFiles )
{
for( Iterator<WeakReference<FileSystemWrapper<?>>> iterator = m_openFiles.keySet().iterator(); iterator.hasNext(); )
{
WeakReference<FileSystemWrapper<?>> reference = iterator.next();
FileSystemWrapper<?> wrapper = reference.get();
if( wrapper == null ) continue;
if( wrapper.mount == mount )
{
wrapper.closeExternally();
iterator.remove();
}
}
}
}
public String combine( String path, String childPath )
@ -371,7 +393,7 @@ public class FileSystem
}
}
private synchronized <T extends Closeable> FileSystemWrapper<T> openFile( @Nonnull Channel channel, @Nonnull T file ) throws FileSystemException
private synchronized <T extends Closeable> FileSystemWrapper<T> openFile( @Nonnull MountWrapper mount, @Nonnull Channel channel, @Nonnull T file ) throws FileSystemException
{
synchronized( m_openFiles )
{
@ -384,13 +406,13 @@ public class FileSystem
}
ChannelWrapper<T> channelWrapper = new ChannelWrapper<>( file, channel );
FileSystemWrapper<T> fsWrapper = new FileSystemWrapper<>( this, channelWrapper, m_openFileQueue );
FileSystemWrapper<T> fsWrapper = new FileSystemWrapper<>( this, mount, channelWrapper, m_openFileQueue );
m_openFiles.put( fsWrapper.self, channelWrapper );
return fsWrapper;
}
}
synchronized void removeFile( FileSystemWrapper<?> handle )
void removeFile( FileSystemWrapper<?> handle )
{
synchronized( m_openFiles )
{
@ -405,11 +427,7 @@ public class FileSystem
path = sanitizePath( path );
MountWrapper mount = getMount( path );
ReadableByteChannel channel = mount.openForRead( path );
if( channel != null )
{
return openFile( channel, open.apply( channel ) );
}
return null;
return channel != null ? openFile( mount, channel, open.apply( channel ) ) : null;
}
public synchronized <T extends Closeable> FileSystemWrapper<T> openForWrite( String path, boolean append, Function<WritableByteChannel, T> open ) throws FileSystemException
@ -419,11 +437,7 @@ public class FileSystem
path = sanitizePath( path );
MountWrapper mount = getMount( path );
WritableByteChannel channel = append ? mount.openForAppend( path ) : mount.openForWrite( path );
if( channel != null )
{
return openFile( channel, open.apply( channel ) );
}
return null;
return channel != null ? openFile( mount, channel, open.apply( channel ) ) : null;
}
public synchronized long getFreeSpace( String path ) throws FileSystemException

22
src/main/java/dan200/computercraft/core/filesystem/FileSystemWrapper.java
View File

@ -5,6 +5,8 @@
*/
package dan200.computercraft.core.filesystem;
import dan200.computercraft.shared.util.IoUtil;
import javax.annotation.Nonnull;
import java.io.Closeable;
import java.io.IOException;
@ -24,15 +26,18 @@ import java.lang.ref.WeakReference;
*
* @param <T> The type of writer or channel to wrap.
*/
public class FileSystemWrapper<T extends Closeable> implements Closeable
public class FileSystemWrapper<T extends Closeable> implements TrackingCloseable
{
private final FileSystem fileSystem;
final MountWrapper mount;
private final ChannelWrapper<T> closeable;
final WeakReference<FileSystemWrapper<?>> self;
private boolean isOpen = true;
FileSystemWrapper( FileSystem fileSystem, ChannelWrapper<T> closeable, ReferenceQueue<FileSystemWrapper<?>> queue )
FileSystemWrapper( FileSystem fileSystem, MountWrapper mount, ChannelWrapper<T> closeable, ReferenceQueue<FileSystemWrapper<?>> queue )
{
this.fileSystem = fileSystem;
this.mount = mount;
this.closeable = closeable;
self = new WeakReference<>( this, queue );
}
@ -40,10 +45,23 @@ public class FileSystemWrapper<T extends Closeable> implements Closeable
@Override
public void close() throws IOException
{
isOpen = false;
fileSystem.removeFile( this );
closeable.close();
}
void closeExternally()
{
isOpen = false;
IoUtil.closeQuietly( closeable );
}
@Override
public boolean isOpen()
{
return isOpen;
}
@Nonnull
public T get()
{

39
src/main/java/dan200/computercraft/core/filesystem/TrackingCloseable.java Normal file
View File

@ -0,0 +1,39 @@
package dan200.computercraft.core.filesystem;
import java.io.Closeable;
import java.io.IOException;
/**
* A {@link Closeable} which knows when it has been closed.
*
* This is a quick (though racey) way of providing more friendly (and more similar to Lua)
* error messages to the user.
*/
public interface TrackingCloseable extends Closeable
{
boolean isOpen();
class Impl implements TrackingCloseable
{
private final Closeable object;
private boolean isOpen = true;
public Impl( Closeable object )
{
this.object = object;
}
@Override
public boolean isOpen()
{
return isOpen;
}
@Override
public void close() throws IOException
{
isOpen = false;
object.close();
}
}
}

28
src/test/java/dan200/computercraft/core/filesystem/FileSystemTest.java
View File

@ -18,10 +18,19 @@ import java.io.IOException;
import java.nio.charset.StandardCharsets;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
public class FileSystemTest
{
private static final File ROOT = new File( "test-files/filesystem" );
private static final long CAPACITY = 1000000;
private static FileSystem mkFs() throws FileSystemException
{
IWritableMount writableMount = new FileMount( ROOT, CAPACITY );
return new FileSystem( "hdd", writableMount );
}
/**
* Ensures writing a file truncates it.
@ -33,8 +42,7 @@ public class FileSystemTest
@Test
public void testWriteTruncates() throws FileSystemException, LuaException, IOException
{
IWritableMount writableMount = new FileMount( ROOT, 1000000 );
FileSystem fs = new FileSystem( "hdd", writableMount );
FileSystem fs = mkFs();
{
FileSystemWrapper<BufferedWriter> writer = fs.openForWrite( "out.txt", false, EncodedWritableHandle::openUtf8 );
@ -54,4 +62,20 @@ public class FileSystemTest
assertEquals( "Tiny line", Files.asCharSource( new File( ROOT, "out.txt" ), StandardCharsets.UTF_8 ).read() );
}
@Test
public void testUnmountCloses() throws FileSystemException
{
FileSystem fs = mkFs();
IWritableMount mount = new FileMount( new File( ROOT, "child" ), CAPACITY );
fs.mountWritable( "disk", "disk", mount );
FileSystemWrapper<BufferedWriter> writer = fs.openForWrite( "disk/out.txt", false, EncodedWritableHandle::openUtf8 );
ObjectWrapper wrapper = new ObjectWrapper( new EncodedWritableHandle( writer.get(), writer ) );
fs.unmount( "disk" );
LuaException err = assertThrows( LuaException.class, () -> wrapper.call( "write", "Tiny line" ) );
assertEquals( "attempt to use a closed file", err.getMessage() );
}
}