Opened 7 years ago

Closed 9 months ago

#4676 closed enhancement (wontfix)

Eliminate "family" field in tor_addr_t

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay, memory, needs-insight, review-group-34
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The tor_addr_t structure has a "family" member that stores AF_INET, AF_INET6, or AF_UNSPEC. So in theory we'd need only 3 bits for it... but in practice, it bumps up the size of the structure by 4-8 bytes, because of alignment issues.

This excess space matters, because we allocate a whole lot of tor_addr_ts. For example, we allocate one for every exit policy entry.

We can save space here by using the "v4-mapped address" trick of RFC4291, and using ::ffff:1.2.3.4 as the representation of 1.2.3.4. For AF_UNSPEC, we can just choose a sentinel value.

Child Tickets

Change History (12)

comment:1 Changed 7 years ago by nickm

Status: newneeds_review

See branch "short_address_rep" in my public repository for an implementation of this.

One thing to consider carefully here is that parsing what looks like an IPv6 address can create a value that gets construed as an IPv4 address or for AF_UNSPEC. (No *valid* IPv6 address will be taken for an one of those, but hostile input can happen.) We must look at all our code that parses potential IPv6 addresses carefully, to make sure that nothing will break because of this.

comment:2 Changed 7 years ago by rransom

00:19 < wanoskarnet> re branch "short_address_rep": any explicitly non inited tor_addr_t structs will have AF_INET6 family, that hides exist family related bugs.

The all-zero IPv6 address is used as in6addr_any by the IPv6 code, so if we use that as a special ‘uninitialized’ value of tor_addr_t, we'll have to do some careful fiddling to be able to specify in6addr_any in certain inputs and pass it to networking functions.

comment:3 Changed 7 years ago by nickm

00:19 < wanoskarnet> re branch "short_address_rep": any explicitly non inited tor_addr_t structs will have AF_INET6 family, that hides exist family related bugs.

I think the answer to that one is going to have to be: always use tor_addr_make_unspec, not memset(0), to initialize tor_addr_t fields.

Special-casing the address [::] seems like a bad move to me.

comment:4 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

I want to do this, but it feels too big for 0.2.3.x right now. :/

comment:5 Changed 6 years ago by nickm

Keywords: tor-relay added

comment:6 Changed 6 years ago by nickm

Component: Tor RelayTor

comment:7 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: unspecified

FWIW, I'm currently scared of this one, since if we forget tor_addr_make_unspec even one time, we wind up with 0.0.0.0 instead. It might make more sense instead to look for places where we allocate far too many tor_addr_t and we would benefit from saving the 4-8 bytes lost to the family field on each.

Alternatively, we could try to find some tricky way to make sure, mechanically, that every tor_addr_t got initialized. But I don't know a good solution on that one.

comment:8 Changed 6 years ago by nickm

(Alternatively, alternatively, maybe special-casing the rep for [::] isn't so bad as I had thought above. Maybe look into that too.)

comment:9 Changed 19 months ago by nickm

Keywords: memory needs-insight added
Severity: Normal

comment:10 Changed 10 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.4.x-final

Move all needs_review tickets without a release to 0.3.4

comment:11 Changed 10 months ago by nickm

Keywords: review-group-34 added

comment:12 Changed 9 months ago by dgoulet

Resolution: wontfix
Status: needs_reviewclosed

Won't happen. We need family at least for unknown values (unspec), we can't narrow it down. The memory gain is minimal but debatable. Please open new ticket if you think this is still relevant.

Note: See TracTickets for help on using tickets.