Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list
On Sun, Apr 06, 2014 at 11:54:59PM +0200, Michael Haggerty wrote: I didn't reproduce it experimentally, though. We should be able to just lk-owner = 0; before the initial strcpy to fix it, I would think. I think that using the owner field to avoid this problem is a bit indirect, so I will soon submit a fix that involves adding a flag to lock_file objects indicating whether the filename field currently contains the name of a file that needs to be deleted. Yeah, I agree that the current code is a bit subtle. I was planning to address this during the tempfile cleanup project (either in GSoC, if it gets a slot, or just doing it myself). But I don't mind if you want to do something in the interim. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list
On 04/01/2014 10:16 PM, Jeff King wrote:
On Tue, Apr 01, 2014 at 05:58:15PM +0200, Michael Haggerty wrote:
diff --git a/lockfile.c b/lockfile.c
index e679e4c..c989f6c 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -130,6 +130,22 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
*/
static const size_t max_path_len = sizeof(lk-filename) - 5;
+if (!lock_file_list) {
+/* One-time initialization */
+sigchain_push_common(remove_lock_file_on_signal);
+atexit(remove_lock_file);
+}
+
+lk-owner = getpid();
+if (!lk-on_list) {
+/* Initialize *lk and add it to lock_file_list: */
+lk-fd = -1;
+lk-on_list = 1;
+lk-filename[0] = 0;
+lk-next = lock_file_list;
+lock_file_list = lk;
+}
Initializing here is good, since we might be interrupted by a signal at
any time. But what about during the locking procedure? We do:
strcpy(lk-filename, path);
if (!(flags LOCK_NODEREF))
resolve_symlink(lk-filename, max_path_len);
strcat(lk-filename, .lock);
So for a moment, lk-filename contains the name of the valuable file we
are locking. If we get a signal at that moment, do we accidentally
delete it in remove_lock_file?
I think the answer is no, because we check lk-owner before deleting,
which will not match our pid (it should generally be zero due to xcalloc
or static initialization, though perhaps we should clear it here).
But that makes me wonder about the case of a reused lock. It will have
lk-owner set from a previous invocation, and would potentially suffer
from this problem. In other words, I think the change you are
introducing does not have the problem, but the existing code does. :-/
Good point. Yes, I agree that this is a problem in the existing code
and that it wasn't improved by my work.
I didn't reproduce it experimentally, though. We should be able to just
lk-owner = 0;
before the initial strcpy to fix it, I would think.
I think that using the owner field to avoid this problem is a bit
indirect, so I will soon submit a fix that involves adding a flag to
lock_file objects indicating whether the filename field currently
contains the name of a file that needs to be deleted.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list
Jeff King p...@peff.net writes: So for a moment, lk-filename contains the name of the valuable file we are locking. If we get a signal at that moment, do we accidentally delete it in remove_lock_file? I think the answer is no, because we check lk-owner before deleting, which will not match our pid (it should generally be zero due to xcalloc or static initialization, though perhaps we should clear it here). That generally be zero no longer holds since 2/22, no? But that makes me wonder about the case of a reused lock. It will have lk-owner set from a previous invocation, and would potentially suffer from this problem. In other words, I think the change you are introducing does not have the problem, but the existing code does. :-/ Yes, I tend to agree. I didn't reproduce it experimentally, though. We should be able to just lk-owner = 0; before the initial strcpy to fix it, I would think. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 07/22] lock_file(): always add lock_file object to lock_file_list
It used to be that if locking failed, lock_file() usually did not
register the lock_file object in lock_file_list but sometimes it did.
This confusion was compounded if lock_file() was called via
hold_lock_file_for_append(), which has its own failure modes.
The ambiguity didn't have any ill effects, because lock_file objects
cannot be removed from the lock_file_list anyway. But it is
unnecessary to leave this behavior inconsistent.
So change lock_file() to *always* ensure that the lock_file object is
registered in lock_file_list regardless of whether an error occurs.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
lockfile.c | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index e679e4c..c989f6c 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -130,6 +130,22 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
*/
static const size_t max_path_len = sizeof(lk-filename) - 5;
+ if (!lock_file_list) {
+ /* One-time initialization */
+ sigchain_push_common(remove_lock_file_on_signal);
+ atexit(remove_lock_file);
+ }
+
+ lk-owner = getpid();
+ if (!lk-on_list) {
+ /* Initialize *lk and add it to lock_file_list: */
+ lk-fd = -1;
+ lk-on_list = 1;
+ lk-filename[0] = 0;
+ lk-next = lock_file_list;
+ lock_file_list = lk;
+ }
+
if (strlen(path) = max_path_len)
return -1;
strcpy(lk-filename, path);
@@ -138,16 +154,6 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
strcat(lk-filename, .lock);
lk-fd = open(lk-filename, O_RDWR | O_CREAT | O_EXCL, 0666);
if (0 = lk-fd) {
- if (!lock_file_list) {
- sigchain_push_common(remove_lock_file_on_signal);
- atexit(remove_lock_file);
- }
- lk-owner = getpid();
- if (!lk-on_list) {
- lk-next = lock_file_list;
- lock_file_list = lk;
- lk-on_list = 1;
- }
if (adjust_shared_perm(lk-filename)) {
error(cannot fix permission bits on %s, lk-filename);
rollback_lock_file(lk);
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list
On Tue, Apr 01, 2014 at 05:58:15PM +0200, Michael Haggerty wrote:
diff --git a/lockfile.c b/lockfile.c
index e679e4c..c989f6c 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -130,6 +130,22 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
*/
static const size_t max_path_len = sizeof(lk-filename) - 5;
+ if (!lock_file_list) {
+ /* One-time initialization */
+ sigchain_push_common(remove_lock_file_on_signal);
+ atexit(remove_lock_file);
+ }
+
+ lk-owner = getpid();
+ if (!lk-on_list) {
+ /* Initialize *lk and add it to lock_file_list: */
+ lk-fd = -1;
+ lk-on_list = 1;
+ lk-filename[0] = 0;
+ lk-next = lock_file_list;
+ lock_file_list = lk;
+ }
Initializing here is good, since we might be interrupted by a signal at
any time. But what about during the locking procedure? We do:
strcpy(lk-filename, path);
if (!(flags LOCK_NODEREF))
resolve_symlink(lk-filename, max_path_len);
strcat(lk-filename, .lock);
So for a moment, lk-filename contains the name of the valuable file we
are locking. If we get a signal at that moment, do we accidentally
delete it in remove_lock_file?
I think the answer is no, because we check lk-owner before deleting,
which will not match our pid (it should generally be zero due to xcalloc
or static initialization, though perhaps we should clear it here).
But that makes me wonder about the case of a reused lock. It will have
lk-owner set from a previous invocation, and would potentially suffer
from this problem. In other words, I think the change you are
introducing does not have the problem, but the existing code does. :-/
I didn't reproduce it experimentally, though. We should be able to just
lk-owner = 0;
before the initial strcpy to fix it, I would think.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
