Re: [PATCH v2 18/25] lockfile: avoid transitory invalid states

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Apr 7, 2014, 11:13 UTC
Message-ID: <53428846.7060104@alum.mit.edu>
In-Reply-To: <534242AC.7030908@viscovery.net>

On 04/07/2014 08:16 AM, Johannes Sixt wrote:

Show 18 quoted lines

> Am 4/7/2014 1:34, schrieb Michael Haggerty:
>> Because remove_lock_file() can be called any time by the signal
>> handler, it is important that any lock_file objects that are in the
>> lock_file_list are always in a valid state.  And since lock_file
>> objects are often reused (but are never removed from lock_file_list),
>> that means we have to be careful whenever mutating a lock_file object
>> to always keep it in a well-defined state.
>> ...
>> So, instead of encoding part of the lock_file state in the filename
>> field, add a new bit "LOCK_FLAGS_LOCKFILE_ACTIVE" to flags, and use
>> this bit to distinguish between a lock_file object that is active
>> vs. one that is inactive.  Be careful to set this bit only when
>> filename really contains the name of a file that should be deleted on
>> cleanup.
> 
> Since this flag is primarily for communication between the main code and a
> signal handler, the only safe way is to define the flag as volatile
> sig_atomic_t, not to make it a bit of a larger type!

Thanks for the feedback.  You are obviously right, and I will fix it.

But I have a feeling that this line of thought is going to lead to the signal handler's not being able to do anything. How far can we afford to pursue strict correctness? We have

	struct lock_file {
		struct lock_file *next;
		int fd;
		pid_t owner;
		char on_list;
		char filename[PATH_MAX];
	};

	static struct lock_file *lock_file_list;

The signal handler currently reads

    lock_file_list
    lock_file::next
    lock_file::fd
    lock_file::owner
    lock_file::filename
    *lock_file::filename

and writes lock_file_list. Among other things it calls close(), unlink(), vsnprintf(), and fprintf() (the last two via warning()).

But most of these actions are undefined under the C99 standard:

Show 8 quoted lines

> If the signal occurs other than as the result of calling the abort
> or raise function, the behavior is undefined if the signal handler
> refers to any object with static storage duration other than by
> assigning a value to an object declared as volatile sig_atomic_t, or
> the signal handler calls any function in the standard library other
> than the abort function, the _Exit function, or the signal function
> with the first argument equal to the signal number corresponding to
> the signal that caused the invocation of the handler.

I don't have time to rewrite *all* of Git right now, so how can we get reasonable safety and portability within a feasible amount of work?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/