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.

Sign up for GitHub

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

Jump to bottom

Replace dynamically generated errors with constants #711

Merged
merged 8 commits into from
Sep 5, 2020
Merged

Replace dynamically generated errors with constants #711

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1cf684b
Replace dymanically generated errors with const ones
iamnotacake Sep 1, 2020
de4f0d0
Replace dymanically generated callback errors with const ones
iamnotacake Sep 1, 2020
bec889d
Updates after review
iamnotacake Sep 1, 2020
2505434
Rename ErrOverflow -> ErrOutOfMemory
iamnotacake Sep 1, 2020
f204717
Updates after review
iamnotacake Sep 2, 2020
89d2876
Unify few session errors into ErrMessageSize and ErrMessageData
iamnotacake Sep 2, 2020
f4f0857
Fix typo in changelog, change deprecation version
iamnotacake Sep 2, 2020
2e5a258
Make error messages start with lowercase
iamnotacake Sep 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions gothemis/cell/cell.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,9 @@ import (

// Errors returned by Secure Cell.
var (
ErrGetOutSize = errors.New("Failed to get output size")
ilammy marked this conversation as resolved.
Show resolved Hide resolved
ErrEncryptData = errors.New("Failed to protect data")
ErrDecryptData = errors.New("Failed to unprotect data")
ErrInvalidMode = errors.NewWithCode(errors.InvalidParameter, "invalid Secure Cell mode specified")
ErrMissingKey = errors.NewWithCode(errors.InvalidParameter, "empty symmetric key for Secure Cell")
ErrMissingPassphrase = errors.NewWithCode(errors.InvalidParameter, "empty passphrase for Secure Cell")
Expand Down Expand Up @@ -273,7 +276,7 @@ func (sc *SecureCell) Protect(data []byte, context []byte) ([]byte, []byte, erro
C.int(sc.mode),
&encLen,
&addLen)) {
return nil, nil, errors.New("Failed to get output size")
return nil, nil, ErrGetOutSize
}
if sizeOverflow(encLen) || sizeOverflow(addLen) {
return nil, nil, ErrOverflow
Expand All @@ -299,7 +302,7 @@ func (sc *SecureCell) Protect(data []byte, context []byte) ([]byte, []byte, erro
encLen,
add,
addLen)) {
return nil, nil, errors.New("Failed to protect data")
return nil, nil, ErrEncryptData
}

return encData, addData, nil
Expand Down Expand Up @@ -355,7 +358,7 @@ func (sc *SecureCell) Unprotect(protectedData []byte, additionalData []byte, con
ctxLen,
C.int(sc.mode),
&decLen)) {
return nil, errors.New("Failed to get output size")
return nil, ErrGetOutSize
}
if sizeOverflow(decLen) {
return nil, ErrOverflow
Expand All @@ -373,7 +376,7 @@ func (sc *SecureCell) Unprotect(protectedData []byte, additionalData []byte, con
C.int(sc.mode),
unsafe.Pointer(&decData[0]),
decLen)) {
return nil, errors.New("Failed to unprotect data")
return nil, ErrDecryptData
}

return decData, nil
Expand Down
23 changes: 15 additions & 8 deletions gothemis/compare/compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ const (

// Errors returned by Secure Comparator.
var (
ErrAppendSecret = errors.New("Failed to append secret")
ErrCreateObj = errors.New("Failed to create comparator object")
ilammy marked this conversation as resolved.
Show resolved Hide resolved
ErrDestroyObj = errors.New("Failed to destroy comparator object")
ilammy marked this conversation as resolved.
Show resolved Hide resolved
ErrGetCmpData = errors.New("Failed to get compare data")
ErrGetCmpRes = errors.New("Failed to get compare result")
ErrGetOut = errors.New("Failed to get output")
ErrGetOutSize = errors.New("Failed to get output size")
ErrMissingSecret = errors.NewWithCode(errors.InvalidParameter, "empty secret for Secure Comparator")
ErrMissingData = errors.NewWithCode(errors.InvalidParameter, "empty comparison message for Secure Comparator")
ErrOverflow = errors.NewWithCode(errors.NoMemory, "Secure Comparator cannot allocate enough memory")
Expand All @@ -103,7 +110,7 @@ func sizeOverflow(n C.size_t) bool {
func New() (*SecureCompare, error) {
ctx := C.compare_init()
if nil == ctx {
return nil, errors.New("Failed to create comparator object")
return nil, ErrCreateObj
}

sc := &SecureCompare{ctx}
Expand All @@ -118,7 +125,7 @@ func (sc *SecureCompare) Close() error {
if bool(C.compare_destroy(sc.ctx)) {
sc.ctx = nil
} else {
return errors.New("Failed to destroy comparator object")
return ErrDestroyObj
}
}

Expand All @@ -131,7 +138,7 @@ func (sc *SecureCompare) Append(secret []byte) error {
return ErrMissingSecret
}
if !bool(C.compare_append(sc.ctx, unsafe.Pointer(&secret[0]), C.size_t(len(secret)))) {
return errors.New("Failed to append secret")
return ErrAppendSecret
}

return nil
Expand All @@ -142,7 +149,7 @@ func (sc *SecureCompare) Begin() ([]byte, error) {
var outLen C.size_t

if !bool(C.compare_begin_size(sc.ctx, &outLen)) {
return nil, errors.New("Failed to get output size")
return nil, ErrGetOutSize
}
if sizeOverflow(outLen) {
return nil, ErrOverflow
Expand All @@ -151,7 +158,7 @@ func (sc *SecureCompare) Begin() ([]byte, error) {
out := make([]byte, outLen)

if !bool(C.compare_begin(sc.ctx, unsafe.Pointer(&out[0]), outLen)) {
return nil, errors.New("Failed to get compare data")
return nil, ErrGetCmpData
}

return out, nil
Expand All @@ -167,7 +174,7 @@ func (sc *SecureCompare) Proceed(data []byte) ([]byte, error) {
}

if !bool(C.compare_proceed_size(sc.ctx, unsafe.Pointer(&data[0]), C.size_t(len(data)), &outLen)) {
return nil, errors.New("Failed to get output size")
return nil, ErrGetOutSize
}
if sizeOverflow(outLen) {
return nil, ErrOverflow
Expand All @@ -187,7 +194,7 @@ func (sc *SecureCompare) Proceed(data []byte) ([]byte, error) {
return out, nil
}

return nil, errors.New("Failed to get output")
return nil, ErrGetOut
}

// Result returns the result of the comparison.
Expand All @@ -198,5 +205,5 @@ func (sc *SecureCompare) Result() (int, error) {
return int(res), nil
}

return NotReady, errors.New("Failed to get compare result")
return NotReady, ErrGetCmpRes
}
6 changes: 4 additions & 2 deletions gothemis/keys/keypair.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ const (

// Errors returned by key generation.
var (
ErrGetKeySize = errors.New("Failed to get needed key sizes")
ErrGenKeypair = errors.New("Failed to generate keypair")
ilammy marked this conversation as resolved.
Show resolved Hide resolved
ErrInvalidType = errors.NewWithCode(errors.InvalidParameter, "invalid key type specified")
ErrOverflow = errors.NewWithCode(errors.NoMemory, "key generator cannot allocate enough memory")
)
Expand Down Expand Up @@ -103,7 +105,7 @@ func New(keytype int) (*Keypair, error) {

var privLen, pubLen C.size_t
if !bool(C.get_key_size(C.int(keytype), &privLen, &pubLen)) {
return nil, errors.New("Failed to get needed key sizes")
return nil, ErrGetKeySize
}
if sizeOverflow(privLen) || sizeOverflow(pubLen) {
return nil, ErrOverflow
Expand All @@ -113,7 +115,7 @@ func New(keytype int) (*Keypair, error) {
pub := make([]byte, int(pubLen), int(pubLen))

if !bool(C.gen_keys(C.int(keytype), unsafe.Pointer(&priv[0]), privLen, unsafe.Pointer(&pub[0]), pubLen)) {
return nil, errors.New("Failed to generate keypair")
return nil, ErrGenKeypair
}

return &Keypair{
Expand Down
10 changes: 8 additions & 2 deletions gothemis/keys/symmetric.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ import (
"github.com/cossacklabs/themis/gothemis/errors"
)

// Errors returned by key generation.
var (
ErrGetSymmKeySize = errors.New("Failed to get symmetric key size")
ErrGenSymmKey = errors.New("Failed to generate symmetric key")
ilammy marked this conversation as resolved.
Show resolved Hide resolved
)

// SymmetricKey stores a master key for Secure Cell.
type SymmetricKey struct {
Value []byte
Expand All @@ -34,15 +40,15 @@ type SymmetricKey struct {
func NewSymmetricKey() (*SymmetricKey, error) {
var len C.size_t
if !bool(C.get_sym_key_size(&len)) {
return nil, errors.New("Failed to get symmetric key size")
return nil, ErrGetSymmKeySize
}
if sizeOverflow(len) {
return nil, ErrOverflow
}

key := make([]byte, int(len), int(len))
if !bool(C.gen_sym_key(unsafe.Pointer(&key[0]), len)) {
return nil, errors.New("Failed to generate symmetric key")
return nil, ErrGenSymmKey
}

return &SymmetricKey{Value: key}, nil
Expand Down
18 changes: 12 additions & 6 deletions gothemis/message/message.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ const (

// Errors returned by Secure Message.
var (
ErrEncryptMsg = errors.New("Failed to encrypt message")
ErrDecryptMsg = errors.New("Failed to decrypt message")
ErrSignMsg = errors.New("Failed to sign message")
ErrVerifyMsg = errors.New("Failed to verify message")
ErrProcessMsg = errors.New("Failed to process message")
ilammy marked this conversation as resolved.
Show resolved Hide resolved
ErrGetOutSize = errors.New("Failed to get output size")
ErrMissingMessage = errors.NewWithCode(errors.InvalidParameter, "empty message for Secure Cell")
ErrMissingPublicKey = errors.NewWithCode(errors.InvalidParameter, "empty peer public key for Secure Message")
ErrMissingPrivateKey = errors.NewWithCode(errors.InvalidParameter, "empty private key for Secure Message")
Expand Down Expand Up @@ -135,7 +141,7 @@ func messageProcess(private *keys.PrivateKey, peerPublic *keys.PublicKey, messag
C.size_t(len(message)),
C.int(mode),
&outputLength)) {
return nil, errors.New("Failed to get output size")
return nil, ErrGetOutSize
}
if sizeOverflow(outputLength) {
return nil, ErrOverflow
Expand All @@ -153,15 +159,15 @@ func messageProcess(private *keys.PrivateKey, peerPublic *keys.PublicKey, messag
outputLength)) {
switch mode {
case secureMessageEncrypt:
return nil, errors.New("Failed to encrypt message")
return nil, ErrEncryptMsg
case secureMessageDecrypt:
return nil, errors.New("Failed to decrypt message")
return nil, ErrDecryptMsg
case secureMessageSign:
return nil, errors.New("Failed to sign message")
return nil, ErrSignMsg
case secureMessageVerify:
return nil, errors.New("Failed to verify message")
return nil, ErrVerifyMsg
default:
return nil, errors.New("Failed to process message")
return nil, ErrProcessMsg
}
}

Expand Down
47 changes: 30 additions & 17 deletions gothemis/session/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,23 @@ const (

// Errors returned by Secure Session.
var (
ErrMissingClientID = errors.NewWithCode(errors.InvalidParameter, "empty client ID for Secure Session")
ErrMissingPrivateKey = errors.NewWithCode(errors.InvalidParameter, "empty client private key for Secure Session")
ErrMissingMessage = errors.NewWithCode(errors.InvalidParameter, "empty message for Secure Session")
ErrOverflow = errors.NewWithCode(errors.NoMemory, "Secure Session cannot allocate enough memory")
ErrDestroyObj = errors.New("Failed to destroy secure session object")
ErrCreateObj = errors.New("Failed to create secure session object")
ErrGetRequestSize = errors.New("Failed to get request size")
ErrGenRequest = errors.New("Failed to generate request")
ErrGetWrappedSize = errors.New("Failed to get wrapped size")
ErrWrapData = errors.New("Failed to wrap data")
ErrGetUnwrappedSize = errors.New("Failed to get unwrapped size")
ErrUnwrapData = errors.New("Failed to unwrap data")
ErrCallbackGetUnwrappedSize = errors.NewCallbackError("Failed to get unwraped size (get_public_key_by_id callback error)")
ErrCallbackUnwrapData = errors.NewCallbackError("Failed to unwrap data (get_public_key_by_id callback error)")
ErrCallbackGetRemIDLen = errors.NewCallbackError("Failed to get session remote id length")
ilammy marked this conversation as resolved.
Show resolved Hide resolved
ErrCallbackBadRemIDLen = errors.NewCallbackError("Incorrect remote id length (0)")
ErrCallbackGetRemID = errors.NewCallbackError("Failed to get session remote id")
ErrMissingClientID = errors.NewWithCode(errors.InvalidParameter, "empty client ID for Secure Session")
ErrMissingPrivateKey = errors.NewWithCode(errors.InvalidParameter, "empty client private key for Secure Session")
ErrMissingMessage = errors.NewWithCode(errors.InvalidParameter, "empty message for Secure Session")
ErrOverflow = errors.NewWithCode(errors.NoMemory, "Secure Session cannot allocate enough memory")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not clear what exactly overflow. Memory, int capacity, buffer capacity, amount of requests.... Let's give the name which enough just to read and it will describe what exactly overflowed?)

Copy link
Contributor Author

@iamnotacake iamnotacake Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What about ErrOverflow -> ErrOutOfMemory? Quite common name for such types of errors.

UPD: I guess it was called ErrOverflow because in most all cases this error was returned when sizeOverflow() returned false (which means passed size couldn't fit in 32-bit signed int). BTW, I noticed this func implementation is duplicated few times:

cell/cell.go
235:func sizeOverflow(n C.size_t) bool {

message/message.go
108:func sizeOverflow(n C.size_t) bool {

compare/compare.go
104:func sizeOverflow(n C.size_t) bool {

session/session.go
80:func sizeOverflow(n C.size_t) bool {

keys/keypair.go
129:func sizeOverflow(n C.size_t) bool {

so maybe it's better to implement it once and use some imports

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it is much better. Advice for the future. It's good to search similar cases in the other code of project or in similar projects to not introduce something new and use the most common names/patterns/etc. Probably we used some errors like that in other parts of project)

Copy link
Collaborator

@ilammy ilammy Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, all these checks are for size overflow, but from Go viewpoint this can be treated as out-of-memory error. Themis Core asks Go to allocate a ginormous buffer with a length that can't even be represented with Go types, so Go cannot allocate that much memory.

)

// SessionCallbacks implements a delegate for SecureSession.
Expand Down Expand Up @@ -87,7 +100,7 @@ func New(id []byte, signKey *keys.PrivateKey, callbacks SessionCallbacks) (*Secu
C.size_t(len(signKey.Value)))

if ss.ctx == nil {
return nil, errors.New("Failed to create secure session object")
return nil, ErrCreateObj
}

runtime.SetFinalizer(ss, finalize)
Expand All @@ -101,7 +114,7 @@ func (ss *SecureSession) Close() error {
if bool(C.session_destroy(ss.ctx)) {
ss.ctx = nil
} else {
return errors.New("Failed to destroy secure session object")
return ErrDestroyObj
}
}

Expand Down Expand Up @@ -150,7 +163,7 @@ func (ss *SecureSession) ConnectRequest() ([]byte, error) {

if !bool(C.session_connect_size(ss.ctx,
&reqLen)) {
return nil, errors.New("Failed to get request size")
return nil, ErrGetRequestSize
}
if sizeOverflow(reqLen) {
return nil, ErrOverflow
Expand All @@ -160,7 +173,7 @@ func (ss *SecureSession) ConnectRequest() ([]byte, error) {
if !bool(C.session_connect(&ss.ctx,
unsafe.Pointer(&req[0]),
reqLen)) {
return nil, errors.New("Failed to generate request")
return nil, ErrGenRequest
}

return req, nil
Expand All @@ -178,7 +191,7 @@ func (ss *SecureSession) Wrap(data []byte) ([]byte, error) {
unsafe.Pointer(&data[0]),
C.size_t(len(data)),
&outLen)) {
return nil, errors.New("Failed to get wrapped size")
return nil, ErrGetWrappedSize
}
if sizeOverflow(outLen) {
return nil, ErrOverflow
Expand All @@ -190,7 +203,7 @@ func (ss *SecureSession) Wrap(data []byte) ([]byte, error) {
C.size_t(len(data)),
unsafe.Pointer(&out[0]),
outLen)) {
return nil, errors.New("Failed to wrap data")
return nil, ErrWrapData
}

return out, nil
Expand All @@ -213,9 +226,9 @@ func (ss *SecureSession) Unwrap(data []byte) ([]byte, bool, error) {
case (C.GOTHEMIS_SUCCESS == res) && (0 == outLen):
return nil, false, nil
case (C.GOTHEMIS_SSESSION_GET_PUB_FOR_ID_ERROR == res):
return nil, false, errors.NewCallbackError("Failed to get unwraped size (get_public_key_by_id callback error)")
return nil, false, ErrCallbackGetUnwrappedSize
ilammy marked this conversation as resolved.
Show resolved Hide resolved
case (C.GOTHEMIS_BUFFER_TOO_SMALL != res):
return nil, false, errors.New("Failed to get unwrapped size")
return nil, false, ErrGetUnwrappedSize
}
if sizeOverflow(outLen) {
return nil, false, ErrOverflow
Expand All @@ -237,28 +250,28 @@ func (ss *SecureSession) Unwrap(data []byte) ([]byte, bool, error) {
case (C.GOTHEMIS_SUCCESS == res) && (0 < outLen):
return out, false, nil
case (C.GOTHEMIS_SSESSION_GET_PUB_FOR_ID_ERROR == res):
return nil, false, errors.NewCallbackError("Failed to unwrap data (get_public_key_by_id callback error)")
return nil, false, ErrCallbackUnwrapData
}

return nil, false, errors.New("Failed to unwrap data")
return nil, false, ErrUnwrapData
}

// GetRemoteID returns ID of the remote peer.
func (ss *SecureSession) GetRemoteID() ([]byte, error) {
// secure_session_get_remote_id
var outLength C.size_t
if C.secure_session_get_remote_id(ss.ctx.session, nil, &outLength) != C.THEMIS_BUFFER_TOO_SMALL {
return nil, errors.NewCallbackError("Failed to get session remote id length")
return nil, ErrCallbackGetRemIDLen
}
if outLength == 0 {
return nil, errors.NewCallbackError("Incorrect remote id length (0)")
return nil, ErrCallbackBadRemIDLen
}
if sizeOverflow(outLength) {
return nil, ErrOverflow
}
out := make([]byte, int(outLength))
if C.secure_session_get_remote_id(ss.ctx.session, (*C.uint8_t)(&out[0]), &outLength) != C.THEMIS_SUCCESS {
return nil, errors.NewCallbackError("Failed to get session remote id")
return nil, ErrCallbackGetRemID
}
return out, nil
}
Expand Down