@matheusbsbs / matheus-bagliano / commits / 707103fdfd

lockfile: avoid transitory invalid states

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. This was formerly not the case, because part of the state was encoded by setting lk->filename to the empty string vs. a valid filename. It is wrong to assume that this string can be updated atomically; for example, even strcpy(lk->filename, value) is unsafe. But the old code was even more reckless; for example, strcpy(lk->filename, path); if (!(flags & LOCK_NODEREF)) resolve_symlink(lk->filename, max_path_len); strcat(lk->filename, ".lock"); During the call to resolve_symlink(), lk->filename contained the name of the file that was being locked, not the name of the lockfile. If a signal were raised during that interval, then the signal handler would have deleted the valuable file! We could probably continue to use the filename field to encode the state by being careful to write characters 1..N-1 of the filename first, and then overwrite the NUL at filename[0] with the first character of the filename, but that would be awkward and error-prone. So, instead of using the filename field to determine whether the lock_file object is active, add a new field "lock_file::active" for this purpose. Be careful to set this field only when filename really contains the name of a file that should be deleted on cleanup. Helped-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Michael Haggerty committed Oct 1, 2014 at 12:28 UTC 707103fdfd0c03511fa547d9b80638d8160f1a88

3 files changed +28 -11

cache.h

@@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct

576

577 struct lock_file {

578 struct lock_file *next;

579 + volatile sig_atomic_t active;

580 int fd;

581 pid_t owner;

582 char on_list;

lockfile.c

+26 -11

@@ -23,7 +23,7 @@

23 * used to prevent a forked process from closing a lockfile created by

24 * its parent.

25 *

26 - * A lock_file object can be in several states:

26 + * The possible states of a lock_file object are as follows:

27 *

28 * - Uninitialized. In this state the object's on_list field must be

29 * zero but the rest of its contents need not be initialized. As

@@ -32,19 +32,27 @@

32 *

33 * - Locked, lockfile open (after hold_lock_file_for_update(),

34 * hold_lock_file_for_append(), or reopen_lock_file()). In this

35 - * state, the lockfile exists, filename holds the filename of the

36 - * lockfile, fd holds a file descriptor open for writing to the

37 - * lockfile, and owner holds the PID of the process that locked the

38 - * file.

35 + * state:

36 + * - the lockfile exists

37 + * - active is set

38 + * - filename holds the filename of the lockfile

39 + * - fd holds a file descriptor open for writing to the lockfile

40 + * - owner holds the PID of the process that locked the file

41 *

42 * - Locked, lockfile closed (after successful close_lock_file()).

43 * Same as the previous state, except that the lockfile is closed

44 * and fd is -1.

45 *

46 * - Unlocked (after commit_lock_file(), rollback_lock_file(), a

45 - * failed attempt to lock, or a failed close_lock_file()). In this

46 - * state, filename[0] == '\0' and fd is -1. The object is left

47 - * registered in the lock_file_list, and on_list is set.

47 + * failed attempt to lock, or a failed close_lock_file()). In this

48 + * state:

49 + * - active is unset

50 + * - filename[0] == '\0' (usually, though there are transitory states

51 + * in which this condition doesn't hold). Client code should *not*

52 + * rely on this fact!

53 + * - fd is -1

54 + * - the object is left registered in the lock_file_list, and

55 + * on_list is set.

56 */

58 static struct lock_file *lock_file_list;

@@ -175,9 +183,13 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)

183 atexit(remove_lock_file);

184 }

185

186 + if (lk->active)

187 + die("BUG: cannot lock_file(\"%s\") using active struct lock_file",

188 + path);

189 if (!lk->on_list) {

190 /* Initialize *lk and add it to lock_file_list: */

191 lk->fd = -1;

192 + lk->active = 0;

193 lk->owner = 0;

194 lk->filename[0] = 0;

195 lk->next = lock_file_list;

@@ -199,6 +211,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)

211 return -1;

212 }

213 lk->owner = getpid();

214 + lk->active = 1;

215 if (adjust_shared_perm(lk->filename)) {

216 int save_errno = errno;

217 error("cannot fix permission bits on %s", lk->filename);

@@ -298,7 +311,7 @@ int reopen_lock_file(struct lock_file *lk)

311 {

312 if (0 <= lk->fd)

313 die(_("BUG: reopen a lockfile that is still open"));

301 - if (!lk->filename[0])

314 + if (!lk->active)

315 die(_("BUG: reopen a lockfile that has been committed"));

316 lk->fd = open(lk->filename, O_WRONLY);

317 return lk->fd;

@@ -308,7 +321,7 @@ int commit_lock_file(struct lock_file *lk)

321 {

322 char result_file[PATH_MAX];

323

311 - if (!lk->filename[0])

324 + if (!lk->active)

325 die("BUG: attempt to commit unlocked object");

326

327 if (close_lock_file(lk))

@@ -325,6 +338,7 @@ int commit_lock_file(struct lock_file *lk)

338 return -1;

339 }

340

341 + lk->active = 0;

342 lk->filename[0] = 0;

343 return 0;

344 }

@@ -339,11 +353,12 @@ int hold_locked_index(struct lock_file *lk, int die_on_error)

353

354 void rollback_lock_file(struct lock_file *lk)

355 {

342 - if (!lk->filename[0])

356 + if (!lk->active)

357 return;

358

359 if (!close_lock_file(lk)) {

360 unlink_or_warn(lk->filename);

361 + lk->active = 0;

362 lk->filename[0] = 0;

363 }

364 }

read-cache.c

@@ -2046,6 +2046,7 @@ static int commit_locked_index(struct lock_file *lk)

2046 return -1;

2047 if (rename(lk->filename, alternate_index_output))

2048 return -1;

2049 + lk->active = 0;

2050 lk->filename[0] = 0;

2051 return 0;

2052 } else {