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

Ordering[os.Path] impl is broken. Trivial fix included. #44

Closed
ohamel-softwaresecure opened this issue Aug 26, 2020 · 2 comments · Fixed by #90
Closed

Ordering[os.Path] impl is broken. Trivial fix included. #44

ohamel-softwaresecure opened this issue Aug 26, 2020 · 2 comments · Fixed by #90

Comments

@ohamel-softwaresecure
Copy link

ohamel-softwaresecure commented Aug 26, 2020

implicit val pathOrdering: Ordering[Path] = new Ordering[Path]{
    def compare(x: Path, y: Path): Int = {
      val xSegCount = x.segmentCount
      val ySegCount = y.segmentCount
      if (xSegCount < ySegCount) -1
      else if (xSegCount > ySegCount) 1
      else if (xSegCount == 0 && ySegCount == 0) 0
      else{
        var xSeg = ""
        var ySeg = ""
        var i = -1                      // FIX: `i = 0`    (`xSegCount != 0` due to checks above)
        do{
          i += 1                        // broke here.@  `i == xSegCount == x.segmentCount` 
          xSeg = x.getSegment(i)        // `x.getSegment(x.segmentCount)`  -> out-of-bounds exception
          ySeg = y.getSegment(i)
                                        // FIX: `i += 1` here
        } while (i < xSegCount && xSeg == ySeg)
        if (i == xSegCount) 0
        else Ordering.String.compare(xSeg, ySeg)
      }
    }
  }

Or for { i <- 0 until xSegCount } { val cmp = x.getSegment(i) compare y.getSegment(i); if (cmp != 0) return cmp }.
EDIT: Or use the java.lang.Comparable of java.nio.file.Path...

@lefou
Copy link
Member

lefou commented Jan 30, 2021

Do you have an failing example or test case?

@ohamel-softwaresecure
Copy link
Author

#!/usr/local/bin/amm
import ammonite.ops._
import ammonite.ops.ImplicitWd._
def compare(x: os.Path, y: os.Path): Int = {
  val xSegCount = x.segmentCount
  val ySegCount = y.segmentCount
  if (xSegCount < ySegCount) -1
  else if (xSegCount > ySegCount) 1
  else if (xSegCount == 0 && ySegCount == 0) 0
  else{
    var xSeg = ""
    var ySeg = ""
    var i = -1                      // FIX: `i = 0`    (`xSegCount != 0` due to checks above)
    do{
      i += 1                        // broke here.@  `i == xSegCount == x.segmentCount`
      xSeg = x.getSegment(i)        // `x.getSegment(x.segmentCount)`  -> out-of-bounds exception
      ySeg = y.getSegment(i)
                                    // FIX: `i += 1` here
    } while (i < xSegCount && xSeg == ySeg)
    if (i == xSegCount) 0
    else Ordering.String.compare(xSeg, ySeg)
  }
}

import os.Path.pathOrdering // this is annoying to do. :(
val a: os.Path = os.root / "yo" // distinct by identity but value equal
val b: os.Path = os.root / "yo"
println(implicitly[Ordering[os.Path]].compare(a, b))
compare(a, b)
⋊> ~/projects ◦ ./hi.amm                                                                                                                                                                         
Compiling /home/olivier/projects/hi.amm
^[[Ajava.lang.IllegalArgumentException
  sun.nio.fs.UnixPath.getName(UnixPath.java:336)
  sun.nio.fs.UnixPath.getName(UnixPath.java:43)
  os.Path.getSegment(Path.scala:431)
  os.Path$$anon$1.compare(Path.scala:404)
  os.Path$$anon$1.compare(Path.scala:391)
  ammonite.$file.hi$.<clinit>(hi.amm:28)

lihaoyi added a commit that referenced this issue Dec 10, 2021
Fixes #44

The basic issue is that our loops `i` is being incremented and fed into `getSegment` before the out-of-bounds check. Thus if two paths are identical, it ends up overshooting the end of the sequence of segments, resulting in the error reported. The fix is to order the `i += 1` such that it takes place immediately before the out-of-bounds check, to ensure it is always in bounds when we pass it to `getSegment`

The `i == xSegCount` check at the bottom was also incorrect. Just because we've hit the end of the list doesn't mean the two paths are equal, as the point of the loop is to find the first non-equal string segment so we can compare them. Thus we should compare the two string segments every time: if they are equal and we haven't run out, we loop another time, otherwise we return the result of the comparison

Adds a test case that reproduces the failure on master, passes on this PR

Review by @lefou
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants