DiscIO/VolumeVerifier: Avoid out-of-bounds reads on trimmed Wii ISOs. #10838
Conversation
There was a problem hiding this comment.
There is an alternative solution that would cause the "This disc image is too small and lacks some data" message to trigger instead of the "Integrity check failed for block" message. (In practice the former message will be triggered anyway due to missing data beyond the 4 GiB mark, so for practical purposes the only difference is whether the latter message will be shown.) It is as follows: When if (m_progress + bytes_to_read > m_max_progress) is taken, set group_read to false. (Probably also content_read for consistency, though as far as I know there's no crash associated with that one.)
I think not getting the "Integrity check failed for block" message is cleaner in that the information given to the user is more specific about the problem, but if you do want to go with the approach you currently have, here are my comments on it:
| @@ -1215,8 +1215,10 @@ void VolumeVerifier::Process() | |||
| { | |||
| const u64 block_offset = group.offset + offset_in_group; | |||
|
|
|||
| if (read_succeeded && m_volume.CheckBlockIntegrity( | |||
| block_index, m_data.data() + offset_in_group, group.partition)) | |||
| if (read_succeeded && offset_in_group < byte_increment && | |||
There was a problem hiding this comment.
You should use bytes_to_read or m_data.size() (the two of them have the same value at this point in the code) instead of byte_increment.
The difference is that if the end of a group overlaps with the start of another group (which should never happen in practice, but support for it is implemented just in case), byte_increment doesn't include the overlapping part.
| if (read_succeeded && m_volume.CheckBlockIntegrity( | ||
| block_index, m_data.data() + offset_in_group, group.partition)) | ||
| if (read_succeeded && offset_in_group < byte_increment && | ||
| (byte_increment - offset_in_group) >= VolumeWii::BLOCK_TOTAL_SIZE && |
There was a problem hiding this comment.
Instead of having two different statements that check byte_increment, can't you have just one statement? offset_in_group + VolumeWii::BLOCK_TOTAL_SIZE <= byte_increment
AdmiralCurtiss
force-pushed
the
volume-verifier-out-of-bounds-read
branch
from
fdac762
to
10407cc
Compare
|
Like this? Seems to fix the crash, yes. |
@JosJuice Please check if this makes sense or can be implemented better/differently.