GitLab

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.

Trac:
Username: mansour