Skip to content

phillip/748: resolves #748 where using the -I CLI flag would cause an assertion failure whenever it was run#753

Merged
zakird merged 6 commits intomainfrom
phillip/748
Feb 11, 2024
Merged

phillip/748: resolves #748 where using the -I CLI flag would cause an assertion failure whenever it was run#753
zakird merged 6 commits intomainfrom
phillip/748

Conversation

@phillip-stephens
Copy link
Contributor

@phillip-stephens phillip-stephens commented Dec 12, 2023 •
edited
Loading

Took me longer than it should have to find.

Bug Description

This bug was introduced in commit: d9c0

Prior to this commit, when the user wanted to use a file for a list of IP's (-I), that'd trigger this code in iterator.c.

-       uint64_t num_addrs = blocklist_count_allowed();
-       uint64_t group_min_size = num_addrs;
-       if (zconf.list_of_ips_filename) {
-               log_debug("send", "forcing max group size for compatibility with -I");
-               group_min_size = 0xFFFFFFFF;
-       }

This commit moved/changed this logic to send.c.

+       uint64_t num_addrs = blocklist_count_allowed();
+       if (zconf.list_of_ips_filename) {
+               log_debug("send", "forcing max group size for compatibility with -I");
+               num_addrs = 0xFFFFFFFF;
+       }
+       it = iterator_init(zconf.senders, zconf.shard_num,
+                       zconf.total_shards, num_addrs, zconf.ports->port_count);

The issue is that by setting num_addrs=0xFFFFFFFF, we allow the iterator to generate values 0-2^32. However, our radix tree only has values for the blocklist_count_allowed, or all the IP addresses that are NOT black/blocklisted (looks like ~3.7B values based on Dec 12 14:53:23.952 [DEBUG] constraint: 3702243328 IPs in radix array, 15104 IPs in tree.). Therefore our iterator can generate values that would be outside our radix tree (>3.7B), and therefore violate the assertion here.

Fix

I think we can just remove the setting of the group_min_size and let num_addrs = blocklist_count_allowed() as it is in the base case.
The old logic of setting group_min_size = 0xFFFFFFFF for zconf.list_of_ips_filename is retained [here] (https://github.com/zmap/zmap/blob/main/src/iterator.c#L79) for all cases, including -I since even if we're only scanning 1 port, the value is always >= 2^32. IMO removing this will solve the bug and not have any repercussions.

@phillip-stephens
phillip/748: removed problematic code setting num_addrs to be higher …
48e0227 
…than it should, causing an index out-of-bounds issue
@phillip-stephens phillip-stephens marked this pull request as ready for review December 12, 2023 23:05
@phillip-stephens phillip-stephens linked an issue Dec 15, 2023 that may be closed by this pull request
Latest ZMap seems to break with the -I flag #748
Closed
@zakird zakird self-assigned this Dec 18, 2023
@zakird zakird added this to the ZMap 4.0 milestone Dec 20, 2023
@zakird zakird requested a review from dadrian January 2, 2024 03:40
@zakird
Copy link
Member

zakird commented Jan 2, 2024 •
edited
Loading

This seems reasonable to me to allow the blacklist to set the number of scanned IPs because it already has to be scanning 0/0 for this to be working in the first place and the blacklist should know this like in any other case. @dadrian can you confirm this logically makes sense.

@phillip-stephens can you make sure that there's a safety check that if you're using -I then the user is scanning 0/0? I'm a little bit worried that there could be weird behavior here where we don't actually ensure that the the whole cyclic group / blacklist is being iterated over and checked here.

@zakird zakird assigned dadrian and phillip-stephens and unassigned zakird Jan 4, 2024
@phillip-stephens
Copy link
Contributor Author

phillip-stephens commented Jan 9, 2024

This seems reasonable to me to allow the blacklist to set the number of scanned IPs because it already has to be scanning 0/0 for this to be working in the first place and the blacklist should know this like in any other case. @dadrian can you confirm this logically makes sense.

@phillip-stephens can you make sure that there's a safety check that if you're using -I then the user is scanning 0/0? I'm a little bit worried that there could be weird behavior here where we don't actually ensure that the the whole cyclic group / blacklist is being iterated over and checked here.

Hmm, I'm not sure what you mean. The -I option is used when the user wants to scan a specific set of IP's, given in a file. Ex: -I small-set-of-ips.txt. So what would this safety check do in this case?

@zakird zakird merged commit 9fc3cf6 into main Feb 11, 2024
@zakird zakird deleted the phillip/748 branch February 11, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zakird zakird zakird approved these changes

@dadrian dadrian Awaiting requested review from dadrian

Assignees

@zakird zakird

@dadrian dadrian

Labels

None yet

Projects

None yet

Milestone

ZMap 4.0

Development

Successfully merging this pull request may close these issues.

Latest ZMap seems to break with the -I flag

3 participants

@phillip-stephens @zakird @dadrian