Opened 9 years ago

Closed 9 years ago

Last modified 6 years ago

#2328 closed defect (fixed)

addressmap_get_virtual_address insanity

Reported by: rransom Owned by:
Priority: High Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

From #tor:

2010-12-29 05:13 <doorss> "if (type == RESOLVED_TYPE_IPV4)" "return tor_strdup(buf)" buf inited?

buf is not initialized there.

2010-12-29 05:15 <doorss> addressmap_get_virtual_address() is fragile in result. no documented that null can returned. No one caller waiting for that.

Child Tickets

Change History (14)

comment:1 Changed 9 years ago by rransom

We asked doors to examine that function because chang had reported a crash in addressmap_get_virtual_address with Tor from a recent Git master, libevent 2, and bufferevents disabled.

comment:2 Changed 9 years ago by nickm

Milestone: Tor: 0.2.1.x-final
Status: newneeds_review

Possible fix for this bug (and a related one) in branch bug2328_021 in my public repository. Needs review.

comment:3 in reply to:  2 Changed 9 years ago by rransom

Replying to nickm:

Possible fix for this bug (and a related one) in branch bug2328_021 in my public repository. Needs review.

Your branch doesn't fix the tor_strdup(buf) with buf uninitialized introduced in 22f723e4a3fc32983480c7403af9d7e77a3200ea (on branch master, but not maint-0.2.2 or earlier).

For that matter, your branch doesn't seem to fix anything that could have caused a crash inside addressmap_get_virtual_address, although your fixes are clearly a Good Thing.

comment:4 Changed 9 years ago by cypherpunks

For counting stuff need to fix:

      while ((next_virtual_addr & 0xff) == 0 ||
             (next_virtual_addr & 0xff) == 0xff) {
        ++next_virtual_addr;
      }

Should be --available for every ++next_virtual_addr:

      while ((next_virtual_addr & 0xff) == 0 ||
             (next_virtual_addr & 0xff) == 0xff) {
        ++next_virtual_addr;
        if (! --available) {
          log_warn(LD_CONFIG, "Ran out of virtual addresses!");
          return NULL;
        }
      }

comment:5 Changed 9 years ago by nickm

fix applied to the bug2328_021 branch. Thanks!

comment:6 Changed 9 years ago by nickm

ok, will merge, then clean up master.

comment:7 Changed 9 years ago by nickm

merged bug2328_021 into the appropriate branches, then applied 3bc235d97975dfa17ca6 to fix the strdup issue. Anything else?

comment:8 Changed 9 years ago by cypherpunks

Oh. Seems like not all broken configs was spotted, alas.
if you said "VirtualAddrNetwork 127.0.0.1/32" or something like that. You expected only one virtual address, but after:

      if (!strmap_get(addressmap, fmt_addr32(next_virtual_addr))) {
        ++next_virtual_addr;
        break;
      }

Next addressmap_get_virtual_address() returns "127.0.0.2", next ".3", and so on until ".254" (.255 have a catch by while{}).

comment:9 Changed 9 years ago by cypherpunks

It's not about broken ranges, with default "VirtualAddrNetwork 127.192.0.0/10" you have no limits at all. It can return any IPv4 address till all of them mapped.

comment:10 Changed 9 years ago by nickm

see branch bug2328_more in my public repo

comment:11 in reply to:  10 Changed 9 years ago by rransom

Replying to nickm:

see branch bug2328_more in my public repo

The log_info(LD_CONFIG, "%d addrs available", (int)available) call on line 1198 will spam if Tor runs out of virtual IPv4 addresses -- it should probably be removed, especially since the message it logs will almost always be false.

Other than that, addressmap_get_virtual_address looks reasonable to me now.

comment:12 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

great; merged & closing. thanks again!

comment:13 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:14 Changed 6 years ago by nickm

Milestone: Tor: 0.2.1.x-final

Milestone Tor: 0.2.1.x-final deleted

Note: See TracTickets for help on using tickets.