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

os: race condition between (*os.File).Stat and os.Chdir on windows #13752

Closed
hirochachacha opened this issue Dec 28, 2015 · 20 comments
Closed

os: race condition between (*os.File).Stat and os.Chdir on windows #13752

hirochachacha opened this issue Dec 28, 2015 · 20 comments

Comments

@hirochachacha
Copy link
Contributor

You can reproduce by

package main


import(
    "os"
    "fmt"
)

func main() {
    f, err := os.Open(".")
    if err != nil {
        panic(err)
    }

    fi, err := f.Stat()
    if err != nil {
        panic(err)
    }

    fmt.Println(fi)

    err = os.Chdir("..")
    if err != nil {
        panic(err)
    }

    fi, err = f.Stat()
    if err != nil {
        panic(err)
    }

    fmt.Println(fi)
}

Output is:

&{. {16 {2734709284 30489263} {2684316263 30491099} {2684316263 30491099} 0 4096} {0 0} C:\msys64\home\hiro\work 0 0 0}
&{. {16 {2657275984 30489261} {2713337936 30491099} {2713337936 30491099} 0 8192} {0 0} C:\msys64\home\hiro 0 0 0}

First and second line should be the same.

@gopherbot
Copy link

CL https://golang.org/cl/18181 mentions this issue.

@rsc
Copy link
Contributor

rsc commented Dec 28, 2015

FWIW, it is always nice to include the actual output from the program along with what you expected. I cannot easily reproduce, since I don't have a Windows machine handy. (And maybe my Windows machine behaves differently from yours.)

@rsc rsc added this to the Go1.6Maybe milestone Dec 28, 2015
@hirochachacha
Copy link
Contributor Author

I've edited it.

@alexbrainman
Copy link
Member

First and second line should be the same.

Why?
I don't see where the race is. Can you provide more details, please?
Thank you.

Alex

@hirochachacha
Copy link
Contributor Author

(*os.File).Stat() should return a os.FileInfo of the file we opened,
without dependence on process's current directory.

see https://github.com/golang/go/blob/master/src/os/stat_windows.go#L23

if file.name is given as relative path, the problem is occurred.

see also my CL.

Thank you.

hiro

@alexbrainman
Copy link
Member

the problem is occurred.

What is that problem? I don't see how those internal bits that your CL changes affect anything. Can you point me to an example that demonstrates that problem? Thank you.

Alex

@hirochachacha
Copy link
Contributor Author

Hmm, I don't understand what you mean.

Do you try the code above? What output you get?
Of course, you should not run the code on root directory.

Thank you.

hiro

@alexbrainman
Copy link
Member

Do you try the code above? ...

I didn't not. I disagree with your report. You say "First and second line should be the same.", but I don't see why the two lines shouldn't be different. Your output display internal state of os package. Where does it say that this internal state should be one way or other?

Unless you can demonstarte that this internal state affects something visible to os package user, I don't see how this is a bug.

Alex

@hirochachacha
Copy link
Contributor Author

From my understanding, File.Stat should behave like fstat in the unix world.
At least, I expect such a behavior.

I hope os package provide platform-independent interface.

Although it's a corner case for you, I think this is a bug.

@alexbrainman
Copy link
Member

From my understanding, File.Stat should behave like fstat in the unix world.
At least, I expect such a behavior.

Which File.Stat behavior does not meat your expectations? Please provide an example to demonstrate. Just complaining about File.Stat internal state gets us nowhere. Internal state is not behavior.

Alex

@hirochachacha
Copy link
Contributor Author

I don't care the internal state, too.

I say:

(*os.File).Stat() should return a os.FileInfo of the file we opened,
without dependence on process's current directory. 

Thank you.

hiro

@hirochachacha
Copy link
Contributor Author

Sorry about my poor English.

@alexbrainman
Copy link
Member

I am sorry, but I don't see anything broken here. You don't need to explain me why something is broken, you need to provide a small program to demonstrate broken behaviour. Program that outputs something undocumented or unexpected.

Alex

@hirochachacha
Copy link
Contributor Author

but I don't see anything broken here.

Is this mean, you tried my code above? or another way?
Can you show me your tests and results?

@bradfitz
Copy link
Contributor

@alexbrainman, I understand his bug report at least.

See the CL's test: https://go-review.googlesource.com/#/c/18181/4/src/os/os_windows_test.go

@hirochachacha
Copy link
Contributor Author

Maybe, we talked about the difference between equality and identity...

@alexbrainman
Copy link
Member

@bradfitz, are you saying that it is important that reflect.DeepEqual(fi, fi2) is true in new test in CL 18181? If yes, then why? If no, then what are we fixing here?

Alex

@mattn
Copy link
Member

mattn commented Dec 29, 2015

@alexbrainman

(*os.File).Stat is stat not fstat. And the FileInfo is created from name given Open().

https://github.com/golang/go/blob/master/src/os/stat_windows.go#L23

So if the name is relative path, Chdir make different point to the path.

@bradfitz
Copy link
Contributor

@alexbrainman, maybe not DeepEqual (internals are outside of scope), but os.SameFile(fi, fi2) at least. And if that test passes on Linux, it should pass on Windows. (I haven't tried)

@alexbrainman
Copy link
Member

but os.SameFile(fi, fi2) at least

Yes os.SameFile should work, and it does not. So we have a problem. I'll review CL 18181. Thank you, Brad.

Alex

@golang golang locked and limited conversation to collaborators Dec 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants