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

Rework client to prevent after-Close usage, and support perm at Open #574

Merged
merged 12 commits into from
Feb 12, 2024

Conversation

puellanivis
Copy link
Collaborator

Two things in this:

Comment on lines +69 to +72
// FileMode returns the Mode SFTP file attribute converted to an os.FileMode
func (fs *FileStat) FileMode() os.FileMode {
return toFileMode(fs.Mode)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was located in a different file, which is a little weird, since it operates on FileStat.

client.go Show resolved Hide resolved
client.go Show resolved Hide resolved

mu sync.Mutex
mu sync.RWMutex
handle string
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handle is now protected by mutex.

client.go Outdated Show resolved Hide resolved
packet.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server_integration_test.go Outdated Show resolved Hide resolved
client.dispatchRequest(ch, &sshFxpOpenPacket{
ID: id1,
Path: tmppath,
Pflags: pflags,
})
id2 := client.nextID()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this, so we have the potential for a tighter race possibility.

// so we need to also unconditionally mark this handle as invalid.

handle := f.handle
f.handle = ""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the meat of the fix for double closes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design principle here is that the handle on the server side may be reused after a Close, and thus we should synchronize around invalidating the handle in our struct in order to indicate this closed status.

This way, one cannot “accidentally” also use-after-close something like Read or Write as well.

Comment on lines +607 to +609
case 8:
// words[8] can either have full path or just the filename
bad = !strings.HasSuffix(opWord, "/"+goWord)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♀️ I don’t know what is up about the macOS I’m doing dev on, but openssh is dumping full paths, while our go server is not.

But like, this test was passing for other machines, right? So, ¿ 😕 ?

client.go Outdated
if f.handle == "" {
return 0, os.ErrClosed
}
handle := f.handle // need a local copy to prevent aberrent race detection
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we use f.handle in a sub-goroutine, the access is caught by the race detector as being performed “outside of” the lock that we’re holding in this primary goroutine, which triggers the race condition detector.

So, we need to, in this primary goroutine, extract the f.handle value into a local variable, which we can then access through closure in the sub goroutine. We’re safe to use a copy, because it cannot be updated as long as we’re holding either the read or write lock… which is why it’s an aberrent race detection. It just cannot tell that this sub goroutine’s lifetime is guaranteed to be limited by the primary goroutine.

client.go Outdated
attr, _ := unmarshalAttrs(data)
return fileInfoFromStat(attr, path.Base(p)), nil
attr, _, err := unmarshalAttrs(data)
return fileInfoFromStat(attr, path.Base(p)), err
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should prefer returning either a valid left or valid right, not both.

// so we need to also unconditionally mark this handle as invalid.

handle := f.handle
f.handle = ""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design principle here is that the handle on the server side may be reused after a Close, and thus we should synchronize around invalidating the handle in our struct in order to indicate this closed status.

This way, one cannot “accidentally” also use-after-close something like Read or Write as well.

client.go Show resolved Hide resolved
@puellanivis
Copy link
Collaborator Author

Alright, I feel this is pretty solid now. I’ll be merging it shortly.

@drakkan
Copy link
Collaborator

drakkan commented Feb 9, 2024

Alright, I feel this is pretty solid now. I’ll be merging it shortly.

I plan to do some testing this weekend, but I'm sure I won't find anything wrong. Sounds like a great job, thanks!

@puellanivis
Copy link
Collaborator Author

I plan to do some testing this weekend, but I'm sure I won't find anything wrong. Sounds like a great job, thanks!

I’ll hold off until Monday then. More broad testing is better than the very narrow testing that I’ve done so far.

@drakkan
Copy link
Collaborator

drakkan commented Feb 10, 2024

LGTM, since we are adding support for perm at Open for the server implementation maybe we can also add them in requestFromPacket for the request server, but we can also delay this to a future PR if required.

@puellanivis
Copy link
Collaborator Author

Adding support in requestFromPacket was simple enough. Just a one-liner. Adding deeper support to the MemFile implementation to act as an example would take much more work, so I’ll leave that part for a later PR… or an existing PR. :trollface:

@drakkan
Copy link
Collaborator

drakkan commented Feb 12, 2024

Adding support in requestFromPacket was simple enough. Just a one-liner. Adding deeper support to the MemFile implementation to act as an example would take much more work, so I’ll leave that part for a later PR… or an existing PR. :trollface:

Thank you!

@puellanivis puellanivis merged commit 46d90e3 into master Feb 12, 2024
4 checks passed
@puellanivis puellanivis deleted the ISSUE-572-safer-double-close branch February 12, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants