Bugfix: treesize integer overflows #3963
Conversation
If updating the treesize would result in a negative new treesize, we set the new treesize to zero. This is a workaround for enterprise issue #5783. Until we have found the root cause for the error in treesize propagation.
rhafer
requested review from
a team,
labkode,
ishank011 and
glpatcern
as code owners
| } else { | ||
| newSize = treeSize - uint64(-sizeDiff) | ||
| } | ||
| newSize = treeSize - uint64(-sizeDiff) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I dislike nested if/elses. I think new solution is cleaner in that regard. (just my opinion - not critical)
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
I'd rather prefer to have it in the old if - else with an additional condition in the
sizeDiff < 0branch.
@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. 🤷♂️
Previously SetTreesize stored the treesize as an unsigned 64bit integer (converted to a string) and GetTreesize parsed that string as a signed 64bit integer, which could lead to overflow issues.
phil-davis
changed the title
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.
This PR is mainly a workaround for scenarios where the treesize propagation would result in negative treesize, which then are stored as unsigned integers in the xattrs.
https://github.com/owncloud/enterprise/issues/5783