Skip to content
This repository has been archived by the owner on Jul 15, 2018. It is now read-only.

common/IsDirEmpty: do not mask non-existance errors #34

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

odeke-em
Copy link
Contributor

@odeke-em odeke-em commented Aug 4, 2017

Currently IsDirEmpty returns true, err if it encounters
any error after trying to os.Open the directory.
I noticed this while studying the code and recalled a bug
from an earlier project in which doing the exact same thing
on code without permissions would trip out and falsely report
that the directory was empty.
Given demo.go in https://play.golang.org/p/vhTPU2RiCJ

  • Demo:
$ mkdir -p sample-demo/1 && touch sample-demo/2
$ echo "1st round" && go run demo.go sample-demo
$ sudo chown root sample-demo && sudo chmod 0700 sample-demo
$ echo "2nd round" && go run demo.go sample-demo

That then prints out

1st round
original:: empty: false err: <nil>
updated::  empty: false err: <nil>
2nd round
original:: empty: true err: open data/: permission denied
updated::  empty: false err: open data/: permission denied

where in "2nd round", the original code falsely reports that
the directory is empty but that's a permission error.

I could write a code test for it, but that test requires me to change
users and switch to root as a Go user so no point in complicating our
tests, but otherwise it is a 1-to-1 translation between shell and Go.

Currently IsDirEmpty returns true, err if it encounters
any error after trying to os.Open the directory.
I noticed this while studying the code and recalled a bug
from an earlier project in which doing the exact same thing
on code without permissions would trip out and falsely report
that the directory was empty.
Given demo.go in https://play.golang.org/p/vhTPU2RiCJ

* Demo:
```shell
$ mkdir -p sample-demo/1 && touch sample-demo/2
$ echo "1st round" && go run demo.go sample-demo
$ sudo chown root sample-demo && sudo chmod 0700 sample-demo
$ echo "2nd round" && go run demo.go sample-demo
```

That then prints out
```shell
1st round
original:: empty: false err: <nil>
updated::  empty: false err: <nil>
2nd round
original:: empty: true err: open data/: permission denied
updated::  empty: false err: open data/: permission denied
```

where in "2nd round", the original code falsely reports that
the directory is empty but that's a permission error.

I could write a code test for it, but that test requires me to change
users and switch to root as a Go user so no point in complicating our
tests, but otherwise it is a 1-to-1 translation between shell and Go.
@odeke-em
Copy link
Contributor Author

odeke-em commented Aug 4, 2017

/cc @ethanfrey @ebuchman s'il vous plait.

@ebuchman
Copy link
Contributor

Interesting. I actually can't find a case where we check the error.

Do we think it makes sense to say "the dir is empty" if it doesn't even exist? I guess so long as we're still returning an error. But it means consumers should definitely be checking that error.

Maybe we should remove the function all together and replace it with something more suited to what we use it for. I can only find it being used in basecoin and merkleeyes and for the same thing both times - to see if the database has been initialized!

@ebuchman ebuchman merged commit fe08fc0 into tendermint:develop Aug 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants