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

Bugfix: treesize integer overflows #3963

Merged
merged 2 commits into from
Jun 12, 2023
+28 −7
Merged

Bugfix: treesize integer overflows #3963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
203e6ae
Workaround treesize overflow
rhafer Jun 12, 2023
af1c844
Treat treesize as an unsigned 64bit integer
rhafer Jun 12, 2023
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
7 changes: 7 additions & 0 deletions changelog/unreleased/decomposedfs-treesize-overflow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Bugfix: treesize interger overflows

Reading the treesize was parsing the string value as a signed integer while
setting the treesize used unsigned integers this could cause failures (out of
range errors) when reading very large treesizes.

https://github.com/cs3org/reva/pull/3963
4 changes: 2 additions & 2 deletions pkg/storage/utils/decomposedfs/node/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -948,11 +948,11 @@ func (n *Node) IsDisabled() bool {

// GetTreeSize reads the treesize from the extended attributes
func (n *Node) GetTreeSize() (treesize uint64, err error) {
s, err := n.XattrInt64(prefixes.TreesizeAttr)
s, err := n.XattrUint64(prefixes.TreesizeAttr)
rhafer marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return 0, err
}
return uint64(s), nil
return s, nil
}

// SetTreeSize writes the treesize to the extended attributes
Expand Down
9 changes: 9 additions & 0 deletions pkg/storage/utils/decomposedfs/node/xattrs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,3 +174,12 @@ func (n *Node) XattrInt64(key string) (int64, error) {
}
return strconv.ParseInt(b, 10, 64)
}

// XattrUint64 returns the uint64 representation of an attribute
func (n *Node) XattrUint64(key string) (uint64, error) {
b, err := n.XattrString(key)
if err != nil {
return 0, err
}
return strconv.ParseUint(b, 10, 64)
}
15 changes: 10 additions & 5 deletions pkg/storage/utils/decomposedfs/tree/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,12 +794,17 @@ func (t *Tree) Propagate(ctx context.Context, n *node.Node, sizeDiff int64) (err
}
case err != nil:
return err
case sizeDiff > 0:
newSize = treeSize + uint64(sizeDiff)
case uint64(-sizeDiff) > treeSize:
// The sizeDiff is larger than the current treesize. Which would result in
// a negative new treesize. Something must have gone wrong with the accounting.
// Reset the current treesize to 0.
sublog.Error().Uint64("treeSize", treeSize).Int64("sizeDiff", sizeDiff).
Msg("Error when updating treesize of parent node. Updated treesize < 0. Reestting to 0")
newSize = 0
default:
if sizeDiff > 0 {
newSize = treeSize + uint64(sizeDiff)
} else {
newSize = treeSize - uint64(-sizeDiff)
}
newSize = treeSize - uint64(-sizeDiff)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this case statement makes the code harder to understand. I'd rather prefer to have it in the old if - else with an additional condition in the sizeDiff < 0 branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

I dislike nested if/elses. I think new solution is cleaner in that regard. (just my opinion - not critical)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I also found the nested if inside the switch harder to read. Especially as I would need to add an additional if branch (or turn the nested if into a nested switch ...)

It might be more readable if I turn this into two separate switch commands though. Let me try ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather prefer to have it in the old if - else with an additional condition in the sizeDiff < 0 branch.

@dragotin Played around with this a bit. And adding another branch to the if statement will actually make the linter complain about the ifElseChain that should be turned into a switch statement ;-)

I guess I'll just leave it as is for now. (Splitting up the switch also doesn't help much with readability I think, as it will require an outer if condition. 🤷‍♂️


// update the tree size of the node
Expand Down