CFBitVector: fix multiple correctness bugs#56
Open
DTW-Thalion wants to merge 1 commit into
Open
Conversation
| /* GetBits packs the requested range, most-significant bit first. | ||
| Range (4,8) covers bits 4..11; the set bits 5 and 11 map to range | ||
| offsets 1 and 7 -> 0b01000001 = 0x41. */ | ||
| CFBitVectorGetBits (bv, CFRangeMake(4, 8), out); |
There was a problem hiding this comment.
What should happen when the range is out of bounds or if any index is out of bounds in the other functions? Is this UB or should we expect an exception?
| #include "../CFTesting.h" | ||
|
|
||
| int main (void) | ||
| { |
There was a problem hiding this comment.
Let's also check some other bit vector sizes that are not a multiple of 8, for example size 13, size 0, size 1.
| Range (4,8) covers bits 4..11; the set bits 5 and 11 map to range | ||
| offsets 1 and 7 -> 0b01000001 = 0x41. */ | ||
| CFBitVectorGetBits (bv, CFRangeMake(4, 8), out); | ||
| PASS_CF(out[0] == 0x41, "GetBits packs the requested range."); |
There was a problem hiding this comment.
Also test a range larger than one byte, for example range size 11.
CFBitVector was substantially broken; this fixes the bugs that the new Tests/CFBitVector/general.m exercises: * CFBitVectorBitMask computed 0xFF << n in int arithmetic and right- shifted before truncating to a byte, so any mask with mostSig > 0 was wrong (e.g. setting a single bit set several). Truncate to UInt8 first. * CFBitVectorOperation single-byte mask used BitMask(startBit, startBit + endBit) instead of BitMask(startBit, endBit). * CFBitVectorGetBits had an empty body; implement it (pack the range, most-significant bit first, matching CFBitVectorGetBitAtIndex). * CFBitVectorGetFirstIndexOfBit looped while idx < range.length instead of idx < range.location + range.length. * CFBitVectorGetLastIndexOfBit started at range.location + range.length with condition idx < range.location, so the loop never ran and it always returned kCFNotFound. * Mutable bit vectors never set _byteCount (left at 0 by the count-0 immutable constructor), so CFBitVectorSetAllBits was a no-op and CFBitVectorSetCount lost the byte count on every grow. Track _byteCount in CFBitVectorCreateMutable and CFBitVectorSetCount and zero newly grown storage.
DTW-Thalion
force-pushed
the
fix/cfbitvector-correctness
branch
from
252d15d to
23f8806
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CFBitVectoris substantially broken for mutable vectors and for severalquery operations. This fixes the bugs exercised by the new
Tests/CFBitVector/general.m:CFBitVectorBitMaskcomputed0xFF << ninintarithmetic andright-shifted before truncating to a byte, so any mask with
mostSig > 0was wrong — e.g.
CFBitVectorSetBitAtIndexset several bits instead of one.Truncate to
UInt8first.CFBitVectorOperationsingle-byte mask usedBitMask(startBit, startBit + endBit)instead ofBitMask(startBit, endBit).CFBitVectorGetBitshad an empty body; implemented (packs the range,MSB first, matching
CFBitVectorGetBitAtIndex).CFBitVectorGetFirstIndexOfBitlooped whileidx < range.lengthinstead of
idx < range.location + range.length.CFBitVectorGetLastIndexOfBitstarted atrange.location + range.lengthwith condition
idx < range.location, so the loop never ran and it alwaysreturned
kCFNotFound._byteCountwas never set (left at 0 by the count-0 immutableconstructor), so
CFBitVectorSetAllBitswas a no-op andCFBitVectorSetCountlost the byte count on every grow. Now tracked in
CFBitVectorCreateMutableand
CFBitVectorSetCount, and newly grown storage is zeroed.Test
Tests/CFBitVector/general.mcoversSetCount/SetAllBits, single-bitset/get,
GetFirstIndexOfBit/GetLastIndexOfBitover sub-ranges, andGetBits. It fails on every point above before this change and passes after.