Fix incorrect byte shift in IPv4Address byte-array initializer#754
Open
adityasingh2400 wants to merge 1 commit into
Open
Fix incorrect byte shift in IPv4Address byte-array initializer#754adityasingh2400 wants to merge 1 commit into
adityasingh2400 wants to merge 1 commit into
Conversation
The IPv4Address(_ bytes: [UInt8]) initializer shifted the third octet by 16 bits instead of 8. This OR'd the third octet into the same bit range as the second octet, so the second octet was corrupted and the third was dropped, while bits 8 through 15 were always left zero. For example, decoding [192, 168, 1, 1] produced 192.169.0.1 instead of 192.168.1.1. The error was not caught because the byte property getter (which correctly uses >> 8 for the third octet) had no symmetric test for the initializer, so the two were never checked against each other. The IPv6Address byte-array initializer uses the correct descending shifts. Change the third octet shift to 8 bits so the initializer is the inverse of the bytes property, and add round-trip and invalid-length tests for the byte-array initializer. Signed-off-by: Aditya Singh <adisin650@gmail.com>
9ef211d to
c7055a9
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
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.
The
IPv4Address(_ bytes: [UInt8])initializer in ContainerizationExtras shifts the third octet by 16 bits instead of 8:Because
bytes[2]lands in the same bit range asbytes[1], the second octet gets corrupted by the OR, the third octet is dropped, and bits 8 through 15 are always left zero. Concretely, decoding[192, 168, 1, 1]yields192.169.0.1instead of192.168.1.1, and[18, 52, 86, 120]yields18.118.0.120instead of18.52.86.120.This went unnoticed because the
bytescomputed property getter uses the correct>> 8for the third octet, but there was no test exercising the byte-array initializer, so the encode and decode paths were never checked against each other. The siblingIPv6Address(_ bytes:)initializer uses the correct descending shifts (<< 120, << 112, ... << 8, << 0), which is what the IPv4 version should mirror.The fix changes the third octet shift to 8 bits so the initializer is the exact inverse of the
bytesproperty. I also added two tests to the initializer suite: a valid-input test that asserts both the resultingvalueand thatinit(bytes).bytes == bytesround-trips, and an invalid-length test. The round-trip test fails on the current code and passes with the fix.Verification:
swift test --filter ContainerizationExtrasTestspasses 221 tests in 26 suites (the IPv4Address suite goes from 23 to 25 tests). The new round-trip test fails before the one-line change and passes after.