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

database: Handle FindAncestryAndRollback datastore.Begin() error #829

Merged
merged 1 commit into from
Aug 27, 2019
Merged

database: Handle FindAncestryAndRollback datastore.Begin() error #829

merged 1 commit into from
Aug 27, 2019

Conversation

peacocb
Copy link
Contributor

@peacocb peacocb commented Jul 31, 2019

Do not call rollback() if transaction is not created.

Fixes #828

@jzelinskie jzelinskie added component/database kind/bug things are not as they seem priority/high important functionality labels Jul 31, 2019
if err != nil {
return Ancestry{}, false, err
}

defer tx.Rollback()
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey there! Great working tracking this down; this is a super common mistake in Go.

I only have one tiny nitpick to fix which is to put the defer statement in the same block as the error check:

tx, err := datastore.Begin()
if err != nil {
  return Ancestry{}, false, err
}
defer tx.Rollback()

return tx.FindAncestry(name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem with nitpicks :-) It takes a while to get the feel for others coding styles etc.

(I had to choose between the func above and the func below to determine where to put the defer and I chose the wrong one !)

@@ -22,7 +22,7 @@ import (

"github.com/coreos/clair/pkg/commonerr"
"github.com/coreos/clair/pkg/pagination"
"github.com/deckarep/golang-set"
mapset "github.com/deckarep/golang-set"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change also related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change isn't related to the actual issue per se, it's just that my editor will not save this file without making this change and discussing it within my group we thought we'd leave it in as it helps with code correctness.
However, I am happy to remove it, just trying to get a feel for coding style etc.

Copy link

Choose a reason for hiding this comment

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

Just for reference the editor is just making it more explicit that the mapset package is actually provided by golang-set as it's not necessarily obvious from the name (from 2014: deckarep/golang-set#11). Personally having the declaration there makes the code feel a little more readable, but I can see that it might not want to be associated with this PR.

Do not call rollback() if transaction is not created.

Fixes #828
@jzelinskie jzelinskie merged commit 41c8d9c into quay:master Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug things are not as they seem priority/high important functionality
Development

Successfully merging this pull request may close these issues.

DoS on ancestry POST causes SIGSEGV
4 participants