Skip to content

Conversation

a-mr
Copy link
Contributor

@a-mr a-mr commented Dec 24, 2019

walkDir does not check whether opening directory succeeded or not, existsDir is not enough because it does not check permissions that the directory can be opened.
Partially addresses #10309 & forum https://forum.nim-lang.org/t/4546.
Manually tested on Linux & Windows.

@demotomohiro
Copy link
Contributor

Calling checkDir before calling walkDir is LBYL (Look Before You Leap) as cumulonimbus explained here.

I think adding checkError parameter with default value false to walkDir (and it check errors only when checkError is true) would works without breaking change.
Or add walkDirSafe that is same to walkDir but it check errors.

@a-mr
Copy link
Contributor Author

a-mr commented Dec 25, 2019

Alternatively, Nim can expose a function for zeroing the last error, say clearOsError, and use it in the old fashion:

clearOsError()
for sub in walkDir(dir):
  echo sub
if osLastError() != OSErrorCode(0):
  echo "status: ", osErrorMsg(osLastError())

@Araq
Copy link
Member

Araq commented Dec 29, 2019

  • Add a .since: (1, 1) declaration.
  • Indentation uses 2 spaces everywhere else.

@disruptek
Copy link
Contributor

checkDir just doesn't seem like a good name. How does it differ from checking to see if the directory is traversable?

I'd rather these checks (LBYL) were opt-in versus opt-out.

@narimiran
Copy link
Member

  • Add a .since: (1, 1) declaration.
  • Indentation uses 2 spaces everywhere else.

@a-mr, what's the status of this PR? Is it superseded by #13163? If so, can it be closed? If not, can you make the requested changes?

@alehander92
Copy link
Contributor

ok guys, what is the usage of this? as @demotomohiro's comment seems very correct in that many usages of this (check if can be read, then read it .. oops now it cant be read as something changed) can lead to subtle bugs.
probably this is still useful for stuff like filesystem listings/file managers etc, but still a note in the docs might be good

@a-mr
Copy link
Contributor Author

a-mr commented Jan 23, 2020

Well, I don't understand TOCTOU w.r.t. to directories. Both wiki page and CERT C coding standards only mention TOCTOU of file access.

But it's true that #13163 will supersede this one. checkDir can be easily implemented using #13163.
I close this unless someone thinks it can still be useful.

@a-mr a-mr closed this Jan 23, 2020
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 this pull request may close these issues.

6 participants