Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrumented path based operations using hooks defined in Checker #325

Merged
merged 10 commits into from
Oct 27, 2024
3 changes: 3 additions & 0 deletions build.mill
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ trait MiMaChecks extends Mima {
// this is fine, because ProcessLike is sealed (and its subclasses should be final)
ProblemFilter.exclude[ReversedMissingMethodProblem]("os.ProcessLike.joinPumperThreadsHook")
)
override def mimaExcludeAnnotations: T[Seq[String]] = Seq(
"os.experimental"
)
}

trait OsLibModule
Expand Down
2 changes: 2 additions & 0 deletions os/src-jvm/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ package object os {

val sub: SubPath = SubPath.sub

val checker: DynamicVariable[Checker] = new DynamicVariable[Checker](Checker.Nop)
Copy link

@dabd dabd Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding these for ergonomics?

def withChecker[T](c: Checker)(op: => T): T = 
  checker.withValue(c)(op)
  
  
def checkReadAccess(path: os.Path): Unit = checker.value.onRead(path)
def checkWriteAccess(path: os.Path): Unit = checker.value.onWrite(path)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for now we can punt on these, since it's a pretty advanced feature so being easy to use isn't as important


/**
* Extractor to let you easily pattern match on [[os.Path]]s. Lets you do
*
Expand Down
2 changes: 2 additions & 0 deletions os/src-native/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ package object os {

val sub: SubPath = SubPath.sub

val checker: DynamicVariable[Checker] = new DynamicVariable[Checker](Checker.Nop)

/**
* Extractor to let you easily pattern match on [[os.Path]]s. Lets you do
*
Expand Down
26 changes: 22 additions & 4 deletions os/src/FileOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,12 @@ import scala.util.Try
* ignore the destination if it already exists, using [[os.makeDir.all]]
*/
object makeDir extends Function1[Path, Unit] {
def apply(path: Path): Unit = Files.createDirectory(path.wrapped)
def apply(path: Path): Unit = {
checker.value.onWrite(path)
Files.createDirectory(path.wrapped)
}
def apply(path: Path, perms: PermSet): Unit = {
checker.value.onWrite(path)
Files.createDirectory(
path.wrapped,
PosixFilePermissions.asFileAttribute(perms.toSet())
Expand All @@ -38,6 +42,7 @@ object makeDir extends Function1[Path, Unit] {
object all extends Function1[Path, Unit] {
def apply(path: Path): Unit = apply(path, null, true)
def apply(path: Path, perms: PermSet = null, acceptLinkedDirectory: Boolean = true): Unit = {
checker.value.onWrite(path)
Copy link

@dabd dabd Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original implementation creates any necessary enclosing directories via java.nio.file.Files#createDirectories.

Here the checker is only checking for write access on the full path.

Don't we need to apply it to the enclosing directories as well?

I see this would entail providing a shim for java.nio.file.Files#createDirectories.

// We special case calling makeDir.all on a symlink to a directory;
// normally createDirectories blows up noisily, when really what most
// people would want is for it to succeed since there is a (linked)
Expand Down Expand Up @@ -84,6 +89,8 @@ object move {
atomicMove: Boolean = false,
createFolders: Boolean = false
): Unit = {
checker.value.onWrite(from)
checker.value.onWrite(to)
if (createFolders && to.segmentCount != 0) makeDir.all(to / up)
val opts1 =
if (replaceExisting) Array[CopyOption](StandardCopyOption.REPLACE_EXISTING)
Expand Down Expand Up @@ -176,6 +183,8 @@ object copy {
createFolders: Boolean = false,
mergeFolders: Boolean = false
): Unit = {
checker.value.onRead(from)
checker.value.onWrite(to)
if (createFolders && to.segmentCount != 0) makeDir.all(to / up)
val opts1 =
if (followLinks) Array[CopyOption]()
Expand All @@ -191,18 +200,17 @@ object copy {
s"Can't copy a directory into itself: $to is inside $from"
)

def copyOne(p: Path): file.Path = {
def copyOne(p: Path): Unit = {
val target = to / p.relativeTo(from)
if (mergeFolders && isDir(p, followLinks) && isDir(target, followLinks)) {
// nothing to do
target.wrapped
} else {
Files.copy(p.wrapped, target.wrapped, opts1 ++ opts2 ++ opts3: _*)
}
}

copyOne(from)
if (stat(from, followLinks = followLinks).isDir) walk(from).map(copyOne)
if (stat(from, followLinks = followLinks).isDir) for (p <- walk(from)) copyOne(p)
Comment on lines -194 to +213
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor optimization that eliminates the creation of an unused collection.
This isn't required for the original ticket.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice optimization! Would walk(from).foreach(copyOne) be more idiomatic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to avoid this.

}

/** This overload is only to keep binary compatibility with older os-lib versions. */
Expand Down Expand Up @@ -311,6 +319,7 @@ object copy {
object remove extends Function1[Path, Boolean] {
def apply(target: Path): Boolean = apply(target, false)
def apply(target: Path, checkExists: Boolean = false): Boolean = {
checker.value.onWrite(target)
if (checkExists) {
Files.delete(target.wrapped)
true
Expand All @@ -322,6 +331,7 @@ object remove extends Function1[Path, Boolean] {
object all extends Function1[Path, Unit] {
def apply(target: Path) = {
require(target.segmentCount != 0, s"Cannot remove a root directory: $target")
checker.value.onWrite(target)

val nioTarget = target.wrapped
if (Files.exists(nioTarget, LinkOption.NOFOLLOW_LINKS)) {
Expand Down Expand Up @@ -350,6 +360,8 @@ object exists extends Function1[Path, Boolean] {
*/
object hardlink {
def apply(link: Path, dest: Path) = {
checker.value.onWrite(link)
checker.value.onWrite(dest)
Files.createLink(link.wrapped, dest.wrapped)
}
}
Expand All @@ -359,6 +371,12 @@ object hardlink {
*/
object symlink {
def apply(link: Path, dest: FilePath, perms: PermSet = null): Unit = {
checker.value.onWrite(link)
checker.value.onWrite(dest match {
case p: RelPath => link / RelPath.up / p
case p: SubPath => link / RelPath.up / p
case p: Path => p
})
val permArray: Array[FileAttribute[_]] =
if (perms == null) Array[FileAttribute[_]]()
else Array(PosixFilePermissions.asFileAttribute(perms.toSet()))
Expand Down
27 changes: 27 additions & 0 deletions os/src/Model.scala
Original file line number Diff line number Diff line change
Expand Up @@ -283,3 +283,30 @@ object PosixStatInfo {
)
}
}

/**
* Defines hooks for path based operations.
*
* This, in conjunction with [[checker]], can be used to implement custom checks like
* - restricting an operation to some path(s)
* - logging an operation
*/
@experimental
trait Checker {

/** A hook for a read operation on `path`. */
def onRead(path: ReadablePath): Unit

/** A hook for a write operation on `path`. */
def onWrite(path: Path): Unit
}

@experimental
object Checker {

/** A no-op [[Checker]]. */
object Nop extends Checker {
def onRead(path: ReadablePath): Unit = ()
def onWrite(path: Path): Unit = ()
}
}
7 changes: 6 additions & 1 deletion os/src/PermsOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ object perms extends Function1[Path, PermSet] {
*/
object set {
def apply(p: Path, arg2: PermSet): Unit = {
checker.value.onWrite(p)
Files.setPosixFilePermissions(p.wrapped, arg2.toSet())
}
}
Expand All @@ -44,7 +45,10 @@ object owner extends Function1[Path, UserPrincipal] {
* Set the owner of the file/folder at the given path
*/
object set {
def apply(arg1: Path, arg2: UserPrincipal): Unit = Files.setOwner(arg1.wrapped, arg2)
def apply(arg1: Path, arg2: UserPrincipal): Unit = {
checker.value.onWrite(arg1)
Files.setOwner(arg1.wrapped, arg2)
}
def apply(arg1: Path, arg2: String): Unit = {
apply(
arg1,
Expand Down Expand Up @@ -73,6 +77,7 @@ object group extends Function1[Path, GroupPrincipal] {
*/
object set {
def apply(arg1: Path, arg2: GroupPrincipal): Unit = {
checker.value.onWrite(arg1)
Files.getFileAttributeView(
arg1.wrapped,
classOf[PosixFileAttributeView],
Expand Down
34 changes: 23 additions & 11 deletions os/src/ReadWriteOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ object write {
createFolders: Boolean = false,
openOptions: Seq[OpenOption] = Seq(CREATE, WRITE)
) = {
checker.value.onWrite(target)
if (createFolders) makeDir.all(target / RelPath.up, perms)
if (perms != null && !exists(target)) {
val permArray =
Expand All @@ -53,6 +54,7 @@ object write {
perms: PermSet,
offset: Long
) = {
checker.value.onWrite(target)

import collection.JavaConverters._
val permArray: Array[FileAttribute[_]] =
Expand Down Expand Up @@ -166,6 +168,7 @@ object write {
*/
object channel extends Function1[Path, SeekableByteChannel] {
def write(p: Path, options: Seq[StandardOpenOption]) = {
checker.value.onWrite(p)
java.nio.file.Files.newByteChannel(p.toNIO, options.toArray: _*)
}
def apply(p: Path): SeekableByteChannel = {
Expand Down Expand Up @@ -212,6 +215,7 @@ object write {
*/
object truncate {
def apply(p: Path, size: Long): Unit = {
checker.value.onWrite(p)
val channel = FileChannel.open(p.toNIO, StandardOpenOption.WRITE)
try channel.truncate(size)
finally channel.close()
Expand Down Expand Up @@ -242,16 +246,21 @@ object read extends Function1[ReadablePath, String] {
* Opens a [[java.io.InputStream]] to read from the given file
*/
object inputStream extends Function1[ReadablePath, java.io.InputStream] {
def apply(p: ReadablePath): java.io.InputStream = p.getInputStream
def apply(p: ReadablePath): java.io.InputStream = {
checker.value.onRead(p)
p.getInputStream
}
}

object stream extends Function1[ReadablePath, geny.Readable] {
def apply(p: ReadablePath): geny.Readable = new geny.Readable {
override def contentLength: Option[Long] = p.toSource.contentLength
def readBytesThrough[T](f: java.io.InputStream => T): T = {
val is = p.getInputStream
try f(is)
finally is.close()
def apply(p: ReadablePath): geny.Readable = {
new geny.Readable {
override def contentLength: Option[Long] = p.toSource.contentLength
def readBytesThrough[T](f: java.io.InputStream => T): T = {
val is = os.read.inputStream(p)
try f(is)
finally is.close()
}
}
}
}
Expand All @@ -260,7 +269,10 @@ object read extends Function1[ReadablePath, String] {
* Opens a [[SeekableByteChannel]] to read from the given file.
*/
object channel extends Function1[Path, SeekableByteChannel] {
def apply(p: Path): SeekableByteChannel = p.toSource.getChannel()
def apply(p: Path): SeekableByteChannel = {
checker.value.onRead(p)
p.toSource.getChannel()
}
}

/**
Expand All @@ -271,15 +283,15 @@ object read extends Function1[ReadablePath, String] {
object bytes extends Function1[ReadablePath, Array[Byte]] {
def apply(arg: ReadablePath): Array[Byte] = {
val out = new java.io.ByteArrayOutputStream()
val stream = arg.getInputStream
val stream = os.read.inputStream(arg)
try Internals.transfer(stream, out)
finally stream.close()
out.toByteArray
}
def apply(arg: Path, offset: Long, count: Int): Array[Byte] = {
val arr = new Array[Byte](count)
val buf = ByteBuffer.wrap(arr)
val channel = arg.toSource.getChannel()
val channel = os.read.channel(arg)
try {
channel.position(offset)
val finalCount = channel.read(buf)
Expand Down Expand Up @@ -360,7 +372,7 @@ object read extends Function1[ReadablePath, String] {
def apply(arg: ReadablePath, charSet: Codec) = {
new geny.Generator[String] {
def generate(handleItem: String => Generator.Action) = {
val is = arg.getInputStream
val is = os.read.inputStream(arg)
val isr = new InputStreamReader(is, charSet.decoder)
val buf = new BufferedReader(isr)
var currentAction: Generator.Action = Generator.Continue
Expand Down
1 change: 1 addition & 0 deletions os/src/StatOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ object mtime extends Function1[Path, Long] {
*/
object set {
def apply(p: Path, millis: Long) = {
checker.value.onWrite(p)
Files.setLastModifiedTime(p.wrapped, FileTime.fromMillis(millis))
}
}
Expand Down
9 changes: 6 additions & 3 deletions os/src/TempOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@ object temp {
deleteOnExit: Boolean = true,
perms: PermSet = null
): Path = {
import collection.JavaConverters._
val permArray: Array[FileAttribute[_]] =
if (perms == null) Array.empty
else Array(PosixFilePermissions.asFileAttribute(perms.toSet()))

val nioPath = dir match {
case null => java.nio.file.Files.createTempFile(prefix, suffix, permArray: _*)
case _ => java.nio.file.Files.createTempFile(dir.wrapped, prefix, suffix, permArray: _*)
case _ =>
checker.value.onWrite(dir)
java.nio.file.Files.createTempFile(dir.wrapped, prefix, suffix, permArray: _*)
}

if (contents != null) write.over(Path(nioPath), contents)
Expand Down Expand Up @@ -63,7 +64,9 @@ object temp {

val nioPath = dir match {
case null => java.nio.file.Files.createTempDirectory(prefix, permArray: _*)
case _ => java.nio.file.Files.createTempDirectory(dir.wrapped, prefix, permArray: _*)
case _ =>
checker.value.onWrite(dir)
java.nio.file.Files.createTempDirectory(dir.wrapped, prefix, permArray: _*)
}

if (deleteOnExit) nioPath.toFile.deleteOnExit()
Expand Down
4 changes: 4 additions & 0 deletions os/src/ZipOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ object zip {
deletePatterns: Seq[Regex] = List(),
compressionLevel: Int = java.util.zip.Deflater.DEFAULT_COMPRESSION
): os.Path = {
checker.value.onWrite(dest)
// check read preemptively in case "dest" is created
for (source <- sources) checker.value.onRead(source.src)
Comment on lines +49 to +50
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, the implementation should be refactored to defer file creation until all data is collected.


if (os.exists(dest)) {
val opened = open(dest)
Expand Down Expand Up @@ -268,6 +271,7 @@ object unzip {
excludePatterns: Seq[Regex] = List(),
includePatterns: Seq[Regex] = List()
): Unit = {
checker.value.onWrite(dest)
for ((zipEntry, zipInputStream) <- streamRaw(source, excludePatterns, includePatterns)) {
val newFile = dest / os.SubPath(zipEntry.getName)
if (zipEntry.isDirectory) os.makeDir.all(newFile)
Expand Down
8 changes: 8 additions & 0 deletions os/src/experimental.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package os

import scala.annotation.StaticAnnotation

/**
* Annotation to mark experimental API, which is not guaranteed to stay.
*/
class experimental extends StaticAnnotation {}
1 change: 1 addition & 0 deletions os/test/resources/restricted/File.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
I am a restricted cow
4 changes: 4 additions & 0 deletions os/test/resources/restricted/Multi Line.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
I am restricted cow
Hear me moo
I weigh twice as much as you
And I look good on the barbecue
1 change: 1 addition & 0 deletions os/test/resources/restricted/folder1/one.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Contents of restricted folder one
1 change: 1 addition & 0 deletions os/test/resources/restricted/folder2/nestedA/a.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Contents of restricted nested A
1 change: 1 addition & 0 deletions os/test/resources/restricted/folder2/nestedB/b.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Contents of restricted nested B
Empty file.
1 change: 1 addition & 0 deletions os/test/resources/restricted/misc/file-symlink
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
I am a restricted cow
1 change: 1 addition & 0 deletions os/test/resources/restricted/misc/folder-symlink
Loading