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

ospaths.isAbsolute: uncovering out of bound bugs after updating to 0.18.1 from 0.18.0: empty string and nil string now checked for out of bound errors #8251

Closed
timotheecour opened this issue Jul 8, 2018 · 3 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Jul 8, 2018

was trying to investigate haltcase/glob#15 (I can't reproduce this on Windows.)

Turns out it's because nim 0.18.0 and nim 0.18.1 handle empty and nil strings differently, and nim 0.18.1 correctly throws on out of bound errors.

The issue is that ospaths.isAbsolute doesn't check for out of bound errors in the code: result = path[0] == '/'

  • Should isAbsolute return false (as in dlang std.process.isAbsolute) on nil/empty string or throw? either way, should be documented. My feeling is that throwing sounds saner.

test.nim:

proc test2()=
  try:
    var a = ""
    echo ("a[0]",a[0])
  except Exception as e: echo e[]

  try:
    var a2: string = nil
    echo ("a2[0]",a2[0])
  except Exception as e: echo e[]

nim 0.18.0:

(Field0: "a[0]", Field1: '\x00')
Traceback (most recent call last)
/private/tmp/d06/t49_glob_fail2.nim(43) t49_glob_fail2
/private/tmp/d06/t49_glob_fail2.nim(12) test2
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

nim 0.18.1:

(parent: ...name: IndexError, msg: "index out of bounds", trace: /private/tmp/d06/t49_glob_fail2.nim(43) t49_glob_fail2
/private/tmp/d06/t49_glob_fail2.nim(7) test2
/Users/timothee/.choosenim/toolchains/nim-#devel/lib/system.nim(2871) sysFatal
, raise_id: 0, up: ...)
(parent: ...name: IndexError, msg: "index out of bounds", trace: /private/tmp/d06/t49_glob_fail2.nim(43) t49_glob_fail2
/private/tmp/d06/t49_glob_fail2.nim(12) test2
/Users/timothee/.choosenim/toolchains/nim-#devel/lib/system.nim(2871) sysFatal
, raise_id: 0, up: ...)

=> good, 0.18.1 throws out of bound errors.

@haltcase
Copy link
Contributor

haltcase commented Jul 9, 2018

I don't think it should throw. Equivalent functions in Rust, D, Python, & Node all just return false when given an empty string.

@timotheecour timotheecour changed the title ospaths.isAbsolute: uncovering out of bound bugs after updating to 0.18.1 from 0.18.0: empty string and nil string not now checked for out of bound errors ospaths.isAbsolute: uncovering out of bound bugs after updating to 0.18.1 from 0.18.0: empty string and nil string now checked for out of bound errors Jul 12, 2018
timotheecour added a commit to timotheecour/Nim that referenced this issue Jul 12, 2018
@timotheecour
Copy link
Member Author

sent out #8291

Varriount pushed a commit that referenced this issue Jul 13, 2018
* fix issue #8251 ospaths.isAbsolute: out of bound errors

* address comments

* add reference to a spec for quirky macos paths
@timotheecour
Copy link
Member Author

PR merged, closing this

I don't think it should throw. Equivalent functions in Rust, D, Python, & Node all just return false when given an empty string.

ok, the PR follows that suggestion

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

No branches or pull requests

3 participants