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

Comments

Projects
None yet
4 participants
@dmitshur
Copy link
Member

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

This comment has been minimized.

Copy link
Member

FiloSottile commented Mar 30, 2018

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 30, 2018

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

@gopherbot gopherbot closed this in befd5c4 Mar 30, 2018

@kybin

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

FiloSottile commented Mar 30, 2018

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

@kybin

This comment has been minimized.

Copy link
Contributor

kybin commented Mar 31, 2018

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

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Mar 31, 2018

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. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment