Re: cluts review

[Solar Designer <solar@openwall.com>]

On Thu, Jul 14, 2011 at 12:39:45AM +0400, Solar Designer wrote:
> Rich might be able to provide a better / more correct answer (I'd be
> interested), but here's my understanding:
> 
> The clobbering happens when those variables are kept in registers rather
> than in memory (on stack).  To prevent it from happening, you may force
> the compiler not to place the variables in registers.  One way to do it
> is to take the variable's address:
> 
> (void) &var;
> 
> (I am not sure what guarantees this provides.  IIRC, it was documented
> to provide the needed safety under GNU C.)

I can't find where I think I previously saw this documented for gcc.

> Another is to declare it "volatile", but this does a bit more than is
> needed (so has extra performance impact).

This is documented in lots of places.  For example:

https://www.securecoding.cert.org/confluence/display/seccode/MSC22-C.+Use+the+setjmp(),+longjmp()+facility+securely

and indeed we're taking a risk by potentially interrupting and
re-entering non-async-signal safe functions:

https://www.securecoding.cert.org/confluence/display/seccode/SIG32-C.+Do+not+call+longjmp%28%29+from+inside+a+signal+handler

IIRC, I brought this up before, and Rich correctly pointed out that we
were doing it for low-risk functions only, and this is only a test suite.

> Better yet, structure your function such that there are no variables to
> clobber.  If you put your sigsetjmp() at the very beginning of the
> function, before any local variable is assigned a value, there's nothing
> to clobber yet.

Here's an example:

http://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/popa3d/popa3d/protocol.c?rev=HEAD

int pop_handle_state(struct pop_command *commands)
{
	char line[POP_BUFFER_SIZE];
	char *params;
	struct pop_command *command;
	int response;

	if (sigsetjmp(pop_timed_out, 1)) return POP_CRASH_NETTIME;

	while (pop_get_line(line, sizeof(line))) {
...

It also invokes siglongjmp() from a signal handler, but it only installs
the handler for the duration of a read() syscall, which is async signal
safe, so there's no risk.

Alexander