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

syscall: Dup2 in fd_nacl.go calls files.Unlock more than once #24610

Closed
dmitshur opened this issue Mar 30, 2018 · 6 comments
Closed

syscall: Dup2 in fd_nacl.go calls files.Unlock more than once #24610

dmitshur opened this issue Mar 30, 2018 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Mar 30, 2018

This is an issue on tip, as of this issue creation time. It was originally spotted by @kybin in neelance@0f87113#r27472464 (but not reported here as far as I can tell).

Consider the implementation of Dup2 in src/syscall/fd_nacl.go:

go/src/syscall/fd_nacl.go

Lines 122 to 148 in 4468b0b

func Dup2(fd, newfd int) error {
files.Lock()
defer files.Unlock()
if fd < 0 || fd >= len(files.tab) || files.tab[fd] == nil || newfd < 0 || newfd >= len(files.tab)+100 {
files.Unlock()
return EBADF
}
f := files.tab[fd]
f.fdref++
for cap(files.tab) <= newfd {
files.tab = append(files.tab[:cap(files.tab)], nil)
}
oldf := files.tab[newfd]
var oldfdref int
if oldf != nil {
oldf.fdref--
oldfdref = oldf.fdref
}
files.tab[newfd] = f
files.Unlock()
if oldf != nil {
if oldfdref == 0 {
oldf.impl.close()
}
}
return nil
}

Lines 123 and 124 do files.Lock() and defer files.Unlock().

Lines 126 and 127 do files.Unlock() and return EBADF.

Lines 141 and 147 do files.Unlock() and return nil.

What did you expect to see?

Given that Dup2 executes files.Lock() once at the top, it should execute files.Unlock() exactly once before returning.

What did you see instead?

files.Unlock() is executed twice.

Looking at the other methods, it feels like a copy/paste error. Some methods use defer files.Unlock() only, while other methods use individual files.Unlock() calls before return without using defer files.Unlock(). Dup2 does both.

dmitshur referenced this issue in neelance/go Mar 30, 2018
@FiloSottile
Copy link
Contributor

This indeed breaks on the Playground https://play.golang.org/p/zj2NzfP9iGQ

@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 30, 2018
@gopherbot
Copy link

Change https://golang.org/cl/103775 mentions this issue: syscall: remove double Unlock from Dup2 on nacl

@kybin
Copy link
Contributor

kybin commented Mar 30, 2018

actually I created same cl. but it does not get reviewed.
I should've create issue here first.
anyway it's great to see this fixed.

@FiloSottile
Copy link
Contributor

@kybin Sorry about that. CLs without tracking issues tend to fall off our radar.

@kybin
Copy link
Contributor

kybin commented Mar 31, 2018

@FiloSottile Never mind. I am happy with it. :)

@dmitshur
Copy link
Contributor Author

One small positive aspect of it is the fact that two independent CLs ended up being identical means it’s more likely the fix is optimal. :)

@golang golang locked and limited conversation to collaborators Mar 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants