Skip to content

Tests | Data Races eliminated #91

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

Merged
merged 3 commits into from
Apr 2, 2025
Merged

Conversation

derwesx
Copy link
Contributor

@derwesx derwesx commented Mar 30, 2025

This PR addresses issue #89 by fixing a data race conditions and removing duplicate checks in the test suite.

Changes

  1. Fixed data race in internal/keyring/threadkeyring.go - Added error channel to prevent passage of non-initialized errors between goroutines (Now works with --race flag).
go func() {
	ak, err = keyring.CreateKeyring()
	if err != nil {
		errCh <- err
		return
	}
	errCh <- nil
	...
}

if err <- errCh; ... // Waiting for initialization

return &tk, err 
  1. Fixed data race in cmd/ssh-tpm-agent/main_test.go - launching shell after Stdout is assigned and waiting for the exit.
var b bytes.Buffer
session.Stdout = &b
session.Shell()
session.Wait()
  1. PascalCase + Duplicate checks in internal/keyring/threadkeyring_test.go

Notes

I noticed that keyring.ReadKey appears to return syscall.ENOENT when given unknown key, not syscall.ENOKEY as the test expects.

Also unix.RequestKey(X, description, "", destKeyring) will return ENOENT with X="user" and "logon", whilst ENOKEY with "Asdasdasd", etc.. Seems like an issue with keyring initialization on my system?

_, err = keyring.ReadKey("this.key.does.not.exist") // Returns syscall.ENOENT when tested

assert.Truef(t, errors.Is(err, syscall.ENOKEY), "Got: '%v'\nExpected: '%v'", err, syscall.ENOKEY) 

@Foxboron
Copy link
Owner

I noticed that keyring.ReadKey appears to return syscall.ENOENT when given unknown key, not syscall.ENOKEY as the test expects. Is this behaviour platform-dependent, or should the tests be updated to expect the correct error type?

Uh, the tests have been passing on my machine so I'm not quite sure.

Per the documentation it says that ENOENT should be returned if the keyring doesn't exist, and ENOKEY if the key doesn't exist.

https://docs.kernel.org/security/keys/core.html

Copy link
Owner

@Foxboron Foxboron left a comment

Choose a reason for hiding this comment

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

Please split into two commits.

One for the race condition.

One for the assert changes.

@derwesx derwesx requested a review from Foxboron March 31, 2025 11:05
@derwesx derwesx changed the title [WIP] Keyring Tests | Data Race eliminated, Asserts added Keyring Tests | Data Race eliminated Mar 31, 2025
@derwesx derwesx changed the title Keyring Tests | Data Race eliminated [WIP] Keyring Tests | Data Race eliminated Mar 31, 2025
@derwesx derwesx changed the title [WIP] Keyring Tests | Data Race eliminated [WIP] Tests | Data Races eliminated Mar 31, 2025
@derwesx
Copy link
Contributor Author

derwesx commented Mar 31, 2025

Seems like --race is happy now.

@derwesx derwesx changed the title [WIP] Tests | Data Races eliminated Tests | Data Races eliminated Mar 31, 2025
@Foxboron
Copy link
Owner

Cool, thanks!

There are a couple of things that needs to be done to clean up the commit history.

  • Squash 4a3d707 and f54310d together.
  • Drop the captial casing on each word in the commit, peferably prefix the message with that module you are touching

@derwesx
Copy link
Contributor Author

derwesx commented Mar 31, 2025

Cleaned!

@Foxboron Foxboron merged commit 0eb7353 into Foxboron:master Apr 2, 2025
@Foxboron
Copy link
Owner

Foxboron commented Apr 2, 2025

Thanks!

@Foxboron Foxboron mentioned this pull request Apr 2, 2025
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