util/bitstream: Fix cases where bits would be dropped when reading and writing #12057
Conversation
…f there wasn't enough space in the buffer - util/bitstream: Allow for reading partial bits into the buffer
| // offload more bits if it'll still overflow the buffer | ||
| if (m_bits + numbits >= 32) |
There was a problem hiding this comment.
This is the exact case this is fixing. The first write of 0x0f successfully writes 0x0f, but the second write of 0x0f only writes 0x0e and drops the last bit because it ended up where m_bits = 7 after writing the 3 full 8-bit bytes from m_buffer to m_write, and the number of bits we wanted to write into the buffer was 26 which ends up being 33 bits total, so the last bit was silently dropped.
m_buffer[42047d00] m_bits[29] newbits[0000000f] numbits[26] requiredbits[55] (before offloading to m_write buffer)
m_buffer[00000000] m_bits[ 5] numbits[26] requiredbits[31] (after offloading to m_write buffer)
newbits[000003c0] (after shifting newbits by 32 - numbits)
m_buffer[0000001e] m_bits[31] val[0000000f] (state at end of write func)
m_buffer[0000001e] m_bits[31] newbits[0000000f] numbits[26] requiredbits[57] (before offloading to m_write buffer)
m_buffer[1e000000] m_bits[ 7] numbits[26] requiredbits[33] (after offloading to m_write buffer)
newbits[000003c0] (after shifting newbits by 32 - numbits)
m_buffer[1e000007] m_bits[33] val[0000000e] (state at end of write func)
|
We’re awfully close to a release, so there’s less than 24 hours to make a decision on this. It’s a pretty bad bug as it causes bad CHDs to be produced for large hard disk sizes. I’d appreciate if someone else can eye it over. |
|
I gave it a run through 170 CHDs I had on my HDD and I encountered no immediate issues so I'm marking it ready for review now. For my testing, I ran |
|
I’m merging this. I’d still appreciate other people looking at it. |
|
Commenting this here for visibility. I requested a revert after noticing SHA1s were different when extracting and recreating a lot of the CHDs. I think it's unrelated to this PR after all. The changes seem to happen even with upstream chdman before this PR and the changing SHA1s seem to be a result of other changes from the past. |
|
I created new delta CHDs using Windyfairy's .exes against my problemed kpython2 child CHDs. Just finished and they all pass MAME's audit and verify/info as expected. These were the sets in question, attached:
https://www.emulab.it/forum/index.php?topic=9336.msg26277#msg26277 |
…ing and writing. (#12057) * In some cases, bits would be dropped when writing if there wasn't enough space in the buffer. * Fixes bad hunk maps being written to CHD files and incorrect hunk map data being read.
…ing and writing. (mamedev#12057) * In some cases, bits would be dropped when writing if there wasn't enough space in the buffer. * Fixes bad hunk maps being written to CHD files and incorrect hunk map data being read.
…hen reading and writing. (mamedev#12057)" This reverts commit 69c3cd7. This causes CHD SHA1 digests to change. Either it's buggy, or CHD SHA1 digests depend on the representation rather than the data itself.
…ing and writing. (mamedev#12057) * In some cases, bits would be dropped when writing if there wasn't enough space in the buffer. * Fixes bad hunk maps being written to CHD files and incorrect hunk map data being read.
…ing and writing. (mamedev#12057) * In some cases, bits would be dropped when writing if there wasn't enough space in the buffer. * Fixes bad hunk maps being written to CHD files and incorrect hunk map data being read.
…hen reading and writing. (mamedev#12057)" This reverts commit 69c3cd7. This causes CHD SHA1 digests to change. Either it's buggy, or CHD SHA1 digests depend on the representation rather than the data itself.
…ing and writing. (mamedev#12057) * In some cases, bits would be dropped when writing if there wasn't enough space in the buffer. * Fixes bad hunk maps being written to CHD files and incorrect hunk map data being read.
Additional testing from someone else before merging would be appreciated.
In this case, the parent chunk types were using 26 bits per offset values which behaved unexpectedly resulting in some bits being dropped when reading and writing the data. I think the issue only happens when dealing with values larger than 24 bits due to the way buffering was implemented. So I've changed both
bitstream_inandbitstream_outto more greedily fill its buffers, including partial reads forbitstream_in.Old (can see an additional CRC error message I added to help debug, should be reproducible):
New:
Parent CHDs just to make it easy to verify the metadata of the new child CHD looks correct: