Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#1943 closed defect (fixed)

Helper functions {get,set}_uint{16,32,64}() are not used

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: easy tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The helper functions get_uint{16,32,64}() and set_uint{16,32,64}() should be used instead of "*(uint16_t*)(cp)" etc. to avoid unaligned address access problems with specific OSes.

There are many cases in the source tree that this doesn't happen.
For example, in the cell_unpack() function we have:

dest->command = *(uint8_t*)(src+2);

Child Tickets

Change History (8)

comment:1 Changed 9 years ago by nickm

well, *(uint8_t*)(ptr) is safe: a one-byte read or write is always aligned.

grep '*(uint[136]' src/*/*.c is a good way to look for cases, though not all are problematic. All the ones that touch character buffers should get fixed, even if they wind up being aligned-by-accident. The ones where the memory is aligned-on-purpose but cast to void* so we can pass it to a generic function don't need fixing.

comment:2 Changed 9 years ago by arma

Keywords: easy added
Milestone: Tor: 0.2.2.x-final

Marking as easy, since this is a noninvasive way to get involved in Tor hacking.

My main suggestion would be that for the cases we decide should stay as-is, we should put a comment next to them explaining that "alignment is not an issue here because foo".

comment:3 Changed 9 years ago by arma

Status: newneeds_review

Ok, calling this one easy may have been a false hope. :)

I just pushed a bug1943 branch to my arma.

The main remaining questions are:

a) that instance in geoip.c: is it safe? I think so.

b) all of those calls like *(double *)foo in config.c, in option_clear(), in config_assign_value(), and in get_assigned_option(): they're safe because they elements in the struct are guaranteed to be aligned correctly, yes?

c) I decided to fix the keys and keys+20 instance in circuitbuild.c, even though there shouldn't be any alignment issues, because who knows if someday somebody will pass in keys+2 to the function. And performance isn't a worry there.

d) For cell_pack and cell_unpack, performance could be a worry. Should we leave them fixed, or leave them unclear? I think based on the current struct organization, they should be safe currently.

comment:4 Changed 9 years ago by nickm

a, b) Yes. The objects in question are real honest-to-goodness uint32_ts or doubles. Without loss of genearality, any double that gets actually declared as part of a struct, or on the stack, or wherever, can give you an honest-to-goodness double * when you take its address. Even if you cast the pointer to a void*, it is still "really" a pointer to a double, and even the most whacko of interpretations of C99 let you cast it back to a double* and dereference it.

The place where you get on thin ice is when you cast a void* to a double*, and that void * didn't start life as a double*.

(malloc and its friends are a special case. The output of malloc() is required to be an okay alignment for absolutely anything.)

c) good idea.

d) I think the old code is safe currently on platforms we care about given how our compiler work now, but I'm not willing to call them "safe" in the general case....

But you shouldn't really worry about performance here; the compiler is very smart. Go on, try it: write a quick little test function like:

#include <string.h>
void set_to_short(char*out, unsigned short s)
{
  memcpy(out, &s, 2);
}

and compile it to assembly with gcc -Wall -O2 -s foo.c, and you'll get something like the following in foo.s:

_set:
	pushl	%ebp
	movl	%esp, %ebp
	subl	$4, %esp
	movl	12(%ebp), %edx
	movl	8(%ebp), %eax
	movw	%dx, (%eax)
	leave
	ret

Note that there is no call to memcpy() here; the compiler is smart enough to notice that it is allowed to turn the memcpy into a single movw instruction.

I'm glad to merge this, but is there more? The commit msg implies more is coming.

comment:5 Changed 9 years ago by arma

I don't know of any more. That's part of what wants review.

I just pushed another commit which has a changes file in it.

Merge whenever you like.

comment:6 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merging. My greps don't find any more cases, so I am fairly confident that we got all the easy-to-find ones. Closing.

comment:7 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:8 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.