Fix another hosts.Remove() issue #48
Merged
Conversation
This file contains 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
This reverts commit e336a9e. This keeps the added testcase, but the issue will be fixed differently in one of the following commits. The current fix is fine, even if it's slightly wasteful as it iterates N times over the same line to try to remove hosts entries from it. The main reason for changing it is that this series will fix a second bug, and this will allow to keep consistency between the way we iterate over all the lines, and how we iterate over all the hosts in a given line.
This checks if the host file has the expected content after removing all entries from it.
TestDeleteSliceBoundErrorOnRemove does not need to create many crc-specific entries, the bug can be reproduced with just 2 entries which are deleted in the same call to hosts.Remove(). This commit changes the test to be minimal. It's also renamed to a more descriptive name.
It is the same as TestRemoveMultipleForwardSameLine except that it removes the entries in the opposite order that they are listed in the hosts file.
Removing multiple entries from the same line in one call to hosts.Remove() can currently result in an out-of-bound access and a panic. The removal is done by iterating over the entries, and calling removeHostFromLine with the index of the entry to remove from the line. If the line has for example 2 entries, and we want to remove both, we'll call removeHostFromLine(0) and then removeHostFromLine(1). However, after the first removal, the line no longer has an entry with index 1, which causes the out-of-bound access and the panic when we try to access it. To avoid this problem, we can loop over the line from the end, even if we remove entries, the index of the ones we remove afterwards will still be valid as they are before the ones we removed.
If hosts.Remove() is called with multiple arguments, and the removal triggers the deletion of an empty line, then the removal of subsequent host entries can cause out of bounds accesses, as we access the line by index, and we don't reset our indexes after removing the line. It's a similar issue to the one fixed in "Fix TestRemoveMultiple*SameLine" except that it occurs when iterating over the lines instead of iterating over the hosts on a line. It's fixed in a similar way, by iterating backwards so that even if we remove a line, this does not invalidate the indexes of the lines we'll check after the removed line.
For some reason, when deduplicating the hosts passed to hosts.Remove, we
were first generating a map of bool and then generating a map of
struct{}. The 2 maps would have the same set of keys, corresponding
to the list of hosts with no duplicates.
The second map can be generated directly without generating the
uniqueHosts map first.
While the removal code when there is no crc section does not seem to be impacted by the issues recently fixed, it does not hurt to have a test for it.
This simplifies hosts_test.go code.
evidolob
approved these changes
Dec 19, 2023
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.
This PR reworks the hosts.Remove() fix which was done in 0.5.1 to make it more
efficient, and consistent with a second fix done in this PR.
We have issues with hosts.Remove() both when removing entries from a single
line, but also when the removal of host entries empties some lines and causes
their removal.
0.5.1 fixed the former but not the latter. This PR should fix both, and adds
test cases for the second issue.