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

CreateBucketIfNotExists try opening bucket before creating #504

Closed
wants to merge 1 commit into from
Closed

CreateBucketIfNotExists try opening bucket before creating #504

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 12 additions & 6 deletions bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,13 +190,19 @@ func (b *Bucket) CreateBucket(key []byte) (*Bucket, error) {
// Returns an error if the bucket name is blank, or if the bucket name is too long.
// The bucket instance is only valid for the lifetime of the transaction.
func (b *Bucket) CreateBucketIfNotExists(key []byte) (*Bucket, error) {
child, err := b.CreateBucket(key)
if err == errors.ErrBucketExists {
return b.Bucket(key), nil
} else if err != nil {
return nil, err
if b.tx.db == nil {
return nil, errors.ErrTxClosed
} else if !b.tx.writable {
return nil, errors.ErrTxNotWritable
} else if len(key) == 0 {
return nil, errors.ErrBucketNameRequired
}

child := b.Bucket(key)
Copy link
Member

Choose a reason for hiding this comment

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

Side effect: When the db is opened in readonly mode, the caller can get a valid bucket if present.

The existing implementation will return a ErrTxNotWritable error.

Copy link
Member

Choose a reason for hiding this comment

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

Suggest not to change the behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Calling CreateBucketIfNotExists in read-only mode doesn't make sense. I don't think anybody relied on this behavior.

Copy link
Member

Choose a reason for hiding this comment

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

This PR creates a hole (readonly transaction can even create bucket (actually already exist) ) which might cause unnecessary confusion to users. It can even be regarded as a minor harmless CVE.

Please add the following code at the beginning of the method,

	if b.tx.db == nil {
		return nil, ErrTxClosed
	} else if !b.tx.writable {
		return nil, ErrTxNotWritable
	} else if len(key) == 0 {
		return nil, ErrBucketNameRequired
	}

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also added benchmark results. See PR description. It doesn't help with the operation speed but reduces allocations when there are many buckets which is a valid use cases where buckets are dynamically generated.

if child != nil {
return child, nil
}
return child, nil
return b.CreateBucket(key)
}

// DeleteBucket deletes a bucket at the given key.
Expand Down