Opened 8 years ago

Last modified 2 years ago

#4233 needs_revision enhancement

Exact addition/summation and memory allocation

Reported by: mansour Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: small-feature tor-client memory-safety c correctness
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I've been looking at the memory allocation functions in common/util.c and how they're used.

Sometimes tor_malloc is used like so:

fred = tor_malloc(foo + bar + baz);

That sum could overflow, sometimes to zero -- in which case tor_malloc will return having allocated a single byte. That's to avoid problematic malloc(0) but this behaviour could also be problematic in case of overflow (e.g. if foo bytes are then memcpy-ed). This doesn't happen in practice probably because Tor is well written, but one never knows...

So one solution is to do exact addition/summation before calling tor_malloc. That's the approach I took; here's an example instead of a novel:

size_t size;
tor_assert(tor_sum_sizet(&size, 3, foo, bar, baz) == 3);
fred = tor_malloc(size);

The tor_sum_sizet function stores the sum of foo, bar and baz into the size variable only if there's no overflow. Otherwise size is set to zero and the function returns the number of summands that would have been successfully summed, which should be the same as the second argument passed.

Another thing is cases like:

fred = tor_malloc(foo + 1);

If there is overflow at all it'll be to zero -- so I propose that tor_malloc(0) fails as it would if it were to return NULL. There's no analogue to this in the C standard, but I don't see anywhere in Tor where tor_malloc(0) is used anyway. Also consider:

size_t size;
(void) tor_sum_sizet(&size, 2, foo, bar);
fred = tor_malloc(size);

Here if tor_sum_sizet fails, the size variable is set to 0, and tor_malloc then fails.

I've implemented all the above in common/util.c. Please let me know if this is interesting (perhaps for a distant alpha version) and review if you have time. I'll try to write unit tests for it all.

The last patch (3/3) is an illustration of all the above. I see about 50 places in the code where the same approach could be useful.

Child Tickets

Change History (11)

comment:1 Changed 8 years ago by nickm

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

comment:2 Changed 7 years ago by nickm

Keywords: small-feature added

Tagging with small-feature to let me know I need to review this by the end of the week for it to go in 0.2.3.x

comment:3 Changed 7 years ago by nickm

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

So, the thing about asserting on a tor_malloc(0) seems wrong to me. A sum that could overflow to 0 could as easily overflow to 1, with equally poor consequences.

Safe addition is a good idea; the overflow criteria here are wrong for our case, though, since we treat any value above SIZE_T_CEILING as a probable underflow.

The varargs approach seems error-prone to me, since it can't be typechecked, and seems prone to doing weird things in cases where the arguments have any type besides size_t. (For example, in the single example you give, the value "1" will get passed to the variadic function as an "int," which is signed and potentially smaller than size_t.

Since only one case (and that a safe one) is actually protected by this patch series, I'm going to defer it to the 0.2.4.x merge window.

comment:4 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

comment:5 Changed 7 years ago by nickm

Keywords: tor-client added

comment:6 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:7 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: unspecified

comment:8 Changed 2 years ago by nickm

Keywords: memory-safety c correctness added
Severity: Normal
Note: See TracTickets for help on using tickets.