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

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

Am 4/7/2014 1:34, schrieb Michael Haggerty:

quoted

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:

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/