Signed left-shift undefined behavior in `nbase/nbase_addrset.c` (`(1<<31)`, 4 sites)
Maintainers usually reply within 4 days
Nobody has claimed this yet.
Assessment
- Difficulty
- 1/5
- Estimated time
- Under an hour
- Newbie friendliness
- 85/100
- Issue type
- Bug
- Clarity
- Clearly specified
- Activity status
- Active
- Tech stack
- c
- Domain
- networking, security
Research direction
The issue is in nbase/nbase_addrset.c at lines 235, 393, 582, and 689. Replace each (1<<31) with ((u32)1<<31) as shown in the suggested fix. Build with UBSan to verify the fix, then run a simple scan like ./nmap -n -Pn -sL 127.0.0.1 to ensure no undefined behavior warnings appear.
Written by the indexing model from the issue text.
Description
Summary
nbase/nbase_addrset.c computes the top bit of a 32-bit address with (1<<31),
where 1 is a signed int. Shifting 1 into the sign bit of a 32-bit int is
undefined behavior (C11 §6.5.7 — the result is not representable in int).
-fsanitize=undefined flags it, and it triggers on ordinary runs because the code is
on the address-set matching path. The same file already uses the correct unsigned
idiom ((u32)1 << 31) at lines 408, 624, and 642, so this looks like an oversight in
four sibling sites.
Affected sites
nbase/nbase_addrset.c:235—next_bit_is_one():return ((1<<31) & value);nbase/nbase_addrset.c:393—trie_insert():if ((1<<31) & addr[0]) {nbase/nbase_addrset.c:582—trie_insert_addr():if (mask[0] <= (1<<31)) {nbase/nbase_addrset.c:689—trie_match():if ((1<<31) & addr[0]) {
(Correct usage already in the file: lines 408, 624, 642.)
Reproduction
Build with UBSan and run any scan:
$ CC=clang CXX=clang++ CFLAGS="-g -O1 -fsanitize=address,undefined" \
LDFLAGS="-fsanitize=address,undefined" ./configure --without-zenmap --without-ndiff
$ make -j4 nmap
$ ./nmap -n -Pn -sL 127.0.0.1
nbase_addrset.c:689:9: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
#0 trie_match nbase/nbase_addrset.c:689:9
#1 addrset_contains nbase/nbase_addrset.c:1107:9
#2 HostGroupState::get_next_host targets.cc:466
#3 next_target / refresh_hostbatch / nexthost targets.cc
Observed on master (commit b218bfc, Nmap 7.991SVN), clang 18.1.3, Linux x86_64.
Impact
Undefined behavior only. In practice 1<<31 evaluates to 0x80000000 on mainstream
compilers/ISAs and the surrounding &/<= behaves as intended, so there is no known
functional or security impact today — this is a standards-conformance / portability
fix (and it silences a UBSan finding that otherwise fires on every run).
Suggested fix
Use the unsigned idiom already present elsewhere in the file:
- return ((1<<31) & value);
+ return (((u32)1<<31) & value);
- if ((1<<31) & addr[0]) {
+ if (((u32)1<<31) & addr[0]) {
- if (mask[0] <= (1<<31)) {
+ if (mask[0] <= ((u32)1<<31)) {
- if ((1<<31) & addr[0]) {
+ if (((u32)1<<31) & addr[0]) {
Verified: after applying this, ./nmap -n -Pn -sL 127.0.0.1 produces zero UBSan
runtime errors (previously errored at :689). Happy to open a PR.
Related (not a duplicate)
#1717 fixed a different shift bug in this same file (a shift-count-too-large in
netmask generation, line ~503, architecture-specific). This report is about the
signed (1<<31) shifts into the sign bit at lines 235/393/582/689, which are
undefined regardless of architecture and are still present on current master.
- Dominant language
- C
- Stars
- 13.7k
- Forks
- 2.9k
- PR merge metrics
- No merged PRs in 30d
Getting set up
- No Dockerfile or Docker Compose file
- No pull request template
- Read the contributing guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from nmap/nmap
-
NmapOptionsTest defines test_default_executable twice; the shadowed copy calls a nonexistent assertPossibly taken @I-am-Krish claimed this 34 days ago. Open
Difficulty 2/5 1-3 hours Newbie friendliness 90/100
Maintainers usually reply within 4 days
-
make check-zenmap reports success regardless of test resultsPossibly taken @gargamel778 claimed this 22 days ago. Open
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
Maintainers usually reply within 4 days
-
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
Maintainers usually reply within 4 days
-
Nmap
Difficulty 4/5 3-5 days Newbie friendliness 15/100
Maintainers usually reply within 4 days
-
Nmap
Difficulty 4/5 3-5 days Newbie friendliness 15/100
Maintainers usually reply within 4 days
Similar issues
-
good first issue help wanted
Difficulty 2/5 1-3 hours Newbie friendliness 77/100
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
Maintainers usually reply within 1 day
-
bug good first issue
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
tmewett/BrogueCE#929 · 1 comment ·
Maintainers usually reply within 1 day
-
bug : find_key() compares kty against "ocy" instead of "oct", breaking kid-less HS256 verificationOpen
Difficulty 2/5 1-3 hours Newbie friendliness 77/100
OpenPrinting/cups#1756 ·
Maintainers usually reply within 1 day
-
Difficulty 1/5 1-3 hours Newbie friendliness 76/100
SteamGridDB/SGDBoop#147 ·