Add overflow checks to refcounting, port stuff to Cython etc #891
Conversation
Current coverage is
|
| ((uint32_t) -1) - 1 mark empty entries. | ||
|
|
||
| This, among other things, limits the number of segments in a repository to 2**32-2. This is a hard limit above | ||
| which data corruption will always occur. |
enkore
changed the title
| @@ -155,12 +166,13 @@ cdef class NSKeyIterator: | |||
| cdef class ChunkIndex(IndexBase): | |||
|
|
|||
| value_size = 12 | |||
| max_ref = 2**30-1 # to be *extra* safe, from constants like EMPTY and DELETED | |||
There was a problem hiding this comment.
Why not make this 231-2 to double the space? Also 232-2 should also work it appears since the refcount cannot be negative, so we can always go for the unsigned as the counter, so (unsigned) -1 is deleted, (unsigned)-2 is empty and (unsigned) -3 is "too popular".
This is likely not super critical due to a sheer repetition count, I guess.
|
I tried the current feature branch on a powermac (fedora23), and this is the result: |
|
That looks indeed good. Can you try with 0ac324c whether that still works for BE? Just using INT32_MAX for the ceiling should probably be no problem, but I will have to think some more about it to be sure... [and if we keep that we'll just remove MAX_REF altogether and just write int32_max -- plus, I can then clean up the tests a bit] Naturally all of this breaks for non two's complement, but I doubt that's relevant. |
|
enkore
force-pushed
the
feature/cython-summarize
branch
from
4d43b66 to
b7c6504
Compare
|
Thanks, looks good. I did some cleanup which doesn't affect functionality in b7c6504. Can you copy the cache (~/.borg/cache//, the repo id is in the repository/config file) from a LE machine to your BE machine (and/or vice versa) and check whether e.g. borg info gives you the same output (pass |
|
copied the cache around from Be to LE and from LE to BE and see no problems. |
| raise KeyError(key) | ||
| cdef int32_t refcount = _le32toh(data[0]) | ||
| if refcount != INT32_MAX: | ||
| if INT32_MAX - refcount < 1: |
There was a problem hiding this comment.
refcount is not equal to INT32_MAX (see previous "if"), thus INT32_MAX - refcount can never be 0.
So the "< 1" (which is equivalent to "<= 0" for integers) is misleading as it can never be 0.
So what you really meant here is "< 0"?
When is that the case?
There was a problem hiding this comment.
You're right, refcount != INT32_MAX is already a sufficient check here. It's just the arbitrary additions in _add that need this verbose kind of check. The same applies to decref.
(When MAX_REF != INT32_MAX this would not hold)
There was a problem hiding this comment.
The check is in fact correct, it is needed for corrupted caches from before this change.
So INT32_MAX - (int32_t)-2 (or any other negative number, really) is some negative number.
It's either this or we need to make sure that refcount is > 0 (I don't think there's such a check on load?)
There was a problem hiding this comment.
INT32_MAX - (something negative) is only < 1 for INT32_MIN (because INT32_MIN = -2**31). So if the refcount wrapped around in the past in a cache this wouldn't really work.
There was a problem hiding this comment.
I am not sure I get what you mean, this has nothing to do with INT32_MIN other than
INT32_MAX - -1 == INT32_MIN
INT32_MAX - -2 == INT32_MIN + 1
...
INT32_MAX - -INT32_MIN == 0
All negative numbers (except the last one, but still smaller than 1)
There was a problem hiding this comment.
@enkore The behavior is undefined, but in reality we all know what the end result is.
The problem statement: due to a prior corruption (1.0.[01] on bigendian architectures, integer overflow on very popular chunks before this change was merged, some other sources of corruption) refcount might become negative (ignoring two validly negative values -1 and -2).
We can assume that "this is incorrect refcount, but we are too lazy to calculate correct one, so set it to "really popular" value" - what the current code is doing.
Alternatively the loading method (hashindex_get, except it does not dereference value so not a good fit?) could check the value to be valid (i.e. positive, 0, -1, or -2) and if not - throw the cache away and rebuild or assign a saner "really popular value" or just throw the hands in the air and abort.
I guess there might be other solutions here. But ignoring this condition seems to be the least safe one.
There was a problem hiding this comment.
If we consider this case (and as I wrote above it might be valid to ignore this), then we'd need to do something like
# refcount = -something
if refcount < 0:
refcount = INT32_MAX # aha, now refcount set back to INT32_MAX, ok
elif refcount != INT32_MAX:
refcount += / -= 1 # normal range
# Normal operation: will reach INT32_MAX "naturally" by += 1For _add we'd need to check sign of operands (because both are variable instead of +-1 as here) and choose appropiate check from there. Overflowing is UB and can't be used.
(or just plain cache corruption of some other sort).
There are no integrity checks for the indices, but they should be added.
Alternatively the loading method (hashindex_get, except it does not dereference value so not a good fit?) could check the value to be valid (i.e. positive, 0, -1, or -2) and if not - throw the cache away and rebuild or assign a saner "really popular value" or just throw the hands in the air and abort.
The hashindex on-disk format is just a copy of the memory contents; it's loaded by loading one huge hunk of data into memory; individual entries are not visited when loading/saving from/to disk.
There was a problem hiding this comment.
Note that for the first overflown values (-1, -2) the chunk is lost from the index. So a chunk would only "survive" overflowing by merging, not from incref/decref. Not relevant to the end result.
There was a problem hiding this comment.
nope.
In fact the first overflow value is INT32_MIN ( = INT32_MAX + 1).
the -2 and -1 are the two last possible values after which we revert to 0.
|
Q: If we just changed all int32_t to uint32_t (and adjust the limit to UINT32_MAX**-3** accordingly), then all wrapped around values from previous signed overflows should become valid, and even correct, refcounts w/o requiring extra handling? 126, 127, -128, -127 => 126, 127, 128, 129 |
| unique_size += _le32toh(values[1]) | ||
| unique_csize += _le32toh(values[2]) | ||
| size += <long long> _le32toh(values[1]) * _le32toh(values[0]) | ||
| csize += <long long> _le32toh(values[2]) * _le32toh(values[0]) |
There was a problem hiding this comment.
1 blank too much here after *
|
yes, converting to unsigned like that would work. |
| idx1.decref(H(1)) | ||
| refcount, *_ = idx1[H(1)] | ||
| assert refcount == self.MAX_REF | ||
|
|
There was a problem hiding this comment.
there could be also tests for a normal incref and decref
|
|
as thomas noted independent of what will be applied to 1.0-maint and/or master (which might be different things in the end): this needs very careful testing & review. the chance that somewhere overflows happened is slim, but the chance that we're putting extra bugs in here is higher. since the hashtable is fully deterministic we should be able to verify (by file checksum) that these new algorithms generate the exact same caches as before. but that's just one test of many. |
| if not data: | ||
| raise KeyError(key) | ||
| return _le32toh(data[0]), _le32toh(data[1]) | ||
|
|
||
| def __setitem__(self, key, value): | ||
| assert len(key) == self.key_size | ||
| cdef int32_t[2] data | ||
| cdef uint32_t[2] data | ||
| assert value[0] <= MAX_REF, "maximum number of segments reached" |
There was a problem hiding this comment.
in that case, it is a segment number, not a refcount, so MAX_SEGMENT?
or just have a generic MAX_VALUE?
That was in #887, this changeset only moves that particular fix. |
| # This bytestring was created with 1.0-maint at c2f9533 | ||
| HASHINDEX = b'Gb"0J]a]R$\'Sb#Y:I;r1Ne"V?+Yd"E<ckKQkJHQcG61!$2<RPKGOcMn\\j*@DgUo?TdBsRa4pl.0afm>>Z7O2)/Bb=MbH`\\BZ' \ | ||
| b'7O2)/Bb=MbH`\\BZ7O2)/Bb=MbH`\\BZ7O2)/Bb=MbH`\\BZ7O2)/Bb=MbH`\\BZ7O2)/Bb=MbH`\\BZ7O2)ln@&NF6>#IS_\'+' \ | ||
| b'nR\'Z76]9h.G?%66bQ=X3bAQb=1=dEW%Q=X3bAQb=1=dEW%Q=X3bAQb=1=dEW%Q=X3bAQb=1=dEW%pAaLig9quB' |
There was a problem hiding this comment.
maybe use hex as in other places? or base64 so that it at least doesn't need that many escapes in the value?
There was a problem hiding this comment.
Let's go with b64 (not much difference to a85); hexlify is much longer.
enkore
force-pushed
the
feature/cython-summarize
branch
2 times, most recently
from
f270539 to
19ebe86
Compare
| # This bytestring was created with 1.0-maint at c2f9533 | ||
| HASHINDEX = b'eJzt0L0NgmAUhtHLT0LDEI6AuAEhMVYmVnSuYefC7AB3Aj9KNedJbnfyFne6P67P27w0EdG1Eac+Cm1ZybAsy7Isy7Isy7Isy7I' \ | ||
| b'sy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7LsL9nhc+cqTZ' \ | ||
| b'3XlO2Ys++Du5fX+l1/YFmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVn2/+0O2rYccw==' |
There was a problem hiding this comment.
ZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZVmWZ
sy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Isy7Ls
interesting. frame/chunk length limit of zlib?
|
OK, looks good - merge? |
|
Did someone run the tests on BE recently? Otherwise looks "good to merge" to me as well (w/ squashed fixups). e: btw. can you post how you run the vagrant stuff on ppc on x86? |
|
@enkore i am just booting the ppc qemu. :) |
|
Is there a command line that does that? The whole qemukvmvirtvagrant-thing is a bit of a mystery to me ;) |
|
https://gmplib.org/~tege/qemu.html I used ppc (not: ppc64) and debian 8 ooc. |
|
|
Thank you, my deb8 ppc is still at Installieren des Grundsystems :D |
enkore
force-pushed
the
feature/cython-summarize
branch
from
4ce0aab to
29ebdba
Compare
|
\o/ |
(This changeset does not include #890, since that one is already for 1.0-maint. It supersedes merged #887, and unmerged #888 for master)