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

@hirochachacha hirochachacha commented Dec 28, 2015

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 28, 2015

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

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Contributor Author

@hirochachacha hirochachacha commented Dec 29, 2015

I've edited it.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 29, 2015

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

This comment has been minimized.

Copy link
Contributor Author

@hirochachacha hirochachacha commented Dec 29, 2015

(*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

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 29, 2015

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

This comment has been minimized.

Copy link
Contributor Author

@hirochachacha hirochachacha commented Dec 29, 2015

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

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 29, 2015

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

This comment has been minimized.

Copy link
Contributor Author

@hirochachacha hirochachacha commented Dec 29, 2015

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

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 29, 2015

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

This comment has been minimized.

Copy link
Contributor Author

@hirochachacha hirochachacha commented Dec 29, 2015

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

This comment has been minimized.

Copy link
Contributor Author

@hirochachacha hirochachacha commented Dec 29, 2015

Sorry about my poor English.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 29, 2015

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

This comment has been minimized.

Copy link
Contributor Author

@hirochachacha hirochachacha commented Dec 29, 2015

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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 29, 2015

@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

@mikioh mikioh added the OS-Windows label Dec 29, 2015
@hirochachacha

This comment has been minimized.

Copy link
Contributor Author

@hirochachacha hirochachacha commented Dec 29, 2015

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

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 29, 2015

@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

This comment has been minimized.

Copy link
Member

@mattn 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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 29, 2015

@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

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 29, 2015

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
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.