Re: [PATCH v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
On 09/12/2014 12:15 AM, Ronnie Sahlberg wrote: On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty mhag...@alum.mit.edu wrote: There are a few places that use these values, so define constants for them. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- cache.h| 4 lockfile.c | 11 ++- refs.c | 7 --- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/cache.h b/cache.h index da77094..41d829b 100644 --- a/cache.h +++ b/cache.h @@ -569,6 +569,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st); #define REFRESH_IN_PORCELAIN 0x0020 /* user friendly output, not needs update */ extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg); +/* String appended to a filename to derive the lockfile name: */ +#define LOCK_SUFFIX .lock +#define LOCK_SUFFIX_LEN 5 + struct lock_file { struct lock_file *next; int fd; diff --git a/lockfile.c b/lockfile.c index 964b3fc..bfea333 100644 --- a/lockfile.c +++ b/lockfile.c @@ -176,10 +176,11 @@ static char *resolve_symlink(char *p, size_t s) static int lock_file(struct lock_file *lk, const char *path, int flags) { /* -* subtract 5 from size to make sure there's room for adding -* .lock for the lock file name +* subtract LOCK_SUFFIX_LEN from size to make sure there's +* room for adding .lock for the lock file name: */ - static const size_t max_path_len = sizeof(lk-filename) - 5; + static const size_t max_path_len = sizeof(lk-filename) - + LOCK_SUFFIX_LEN; if (!lock_file_list) { /* One-time initialization */ @@ -204,7 +205,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) strcpy(lk-filename, path); if (!(flags LOCK_NODEREF)) resolve_symlink(lk-filename, max_path_len); - strcat(lk-filename, .lock); + strcat(lk-filename, LOCK_SUFFIX); lk-fd = open(lk-filename, O_RDWR | O_CREAT | O_EXCL, 0666); if (0 = lk-fd) { lk-owner = getpid(); @@ -314,7 +315,7 @@ int commit_lock_file(struct lock_file *lk) if (lk-fd = 0 close_lock_file(lk)) return -1; strcpy(result_file, lk-filename); - i = strlen(result_file) - 5; /* .lock */ + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */ Not a new bug since the previous code is broken too. Should probably checkstrlen(result_file) = 5 here before subtracting 5. Otherwise, a caller that calls commit_lock_file() with an already committed/closed lock_file can cause writing outside the bounds of the array on the line below. Good catch; thanks. I will fix this in the reroll (though probably in a later patch). [...] Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
On 09/12/2014 12:42 AM, Ronnie Sahlberg wrote: Maybe we should not have a public constant defined for the length : +#define LOCK_SUFFIX_LEN 5 since it encourages unsafe code like : (this was unsafe long before your patch so not a regression) + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */ result_file[i] = 0; What about removing LOCK_SUFFIX_LEN from the public API and introduce a helper function something like : /* pointer to the character where the lock suffix starts */ char *lock_suffix_ptr_safe(const char *filename) { size_t len = strlen(filename); if (len 5) die(BUG:... if (strcmp(filename + len - 5, LOCK_SUFFIX) die(BUG:... return filename + len - 5; } and use it instead? At the end of this patch series, LOCK_SUFFIX_LEN is only used in two places outside of lockfile.c: * In check_refname_component(), to ensure that no component of a reference name ends with .lock. This only indirectly has anything to do with lockfiles. * In delete_ref_loose(), to derive the name of the loose reference file from the name of the lockfile. It immediately xmemdupz()s the part of the filename that it needs, so it is kosher. I will add a function get_locked_file_path() for the use of the second caller. I like being able to use the symbolic constant at the first caller, and it is not dangerous. I don't think it is so important to make the constant private, because I think somebody programming sloppily wouldn't be deterred for long by not seeing a symbolic constant for the suffix length. So if it's OK with you I'll leave the constant. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
On Fri, Sep 12, 2014 at 10:13 AM, Michael Haggerty mhag...@alum.mit.edu wrote: On 09/12/2014 12:42 AM, Ronnie Sahlberg wrote: Maybe we should not have a public constant defined for the length : +#define LOCK_SUFFIX_LEN 5 since it encourages unsafe code like : (this was unsafe long before your patch so not a regression) + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */ result_file[i] = 0; What about removing LOCK_SUFFIX_LEN from the public API and introduce a helper function something like : /* pointer to the character where the lock suffix starts */ char *lock_suffix_ptr_safe(const char *filename) { size_t len = strlen(filename); if (len 5) die(BUG:... if (strcmp(filename + len - 5, LOCK_SUFFIX) die(BUG:... return filename + len - 5; } and use it instead? At the end of this patch series, LOCK_SUFFIX_LEN is only used in two places outside of lockfile.c: * In check_refname_component(), to ensure that no component of a reference name ends with .lock. This only indirectly has anything to do with lockfiles. * In delete_ref_loose(), to derive the name of the loose reference file from the name of the lockfile. It immediately xmemdupz()s the part of the filename that it needs, so it is kosher. I will add a function get_locked_file_path() for the use of the second caller. I like being able to use the symbolic constant at the first caller, and it is not dangerous. I don't think it is so important to make the constant private, because I think somebody programming sloppily wouldn't be deterred for long by not seeing a symbolic constant for the suffix length. So if it's OK with you I'll leave the constant. It is OK with me. -- 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 v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty mhag...@alum.mit.edu wrote: There are a few places that use these values, so define constants for them. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- cache.h| 4 lockfile.c | 11 ++- refs.c | 7 --- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/cache.h b/cache.h index da77094..41d829b 100644 --- a/cache.h +++ b/cache.h @@ -569,6 +569,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st); #define REFRESH_IN_PORCELAIN 0x0020 /* user friendly output, not needs update */ extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg); +/* String appended to a filename to derive the lockfile name: */ +#define LOCK_SUFFIX .lock +#define LOCK_SUFFIX_LEN 5 + struct lock_file { struct lock_file *next; int fd; diff --git a/lockfile.c b/lockfile.c index 964b3fc..bfea333 100644 --- a/lockfile.c +++ b/lockfile.c @@ -176,10 +176,11 @@ static char *resolve_symlink(char *p, size_t s) static int lock_file(struct lock_file *lk, const char *path, int flags) { /* -* subtract 5 from size to make sure there's room for adding -* .lock for the lock file name +* subtract LOCK_SUFFIX_LEN from size to make sure there's +* room for adding .lock for the lock file name: */ - static const size_t max_path_len = sizeof(lk-filename) - 5; + static const size_t max_path_len = sizeof(lk-filename) - + LOCK_SUFFIX_LEN; if (!lock_file_list) { /* One-time initialization */ @@ -204,7 +205,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) strcpy(lk-filename, path); if (!(flags LOCK_NODEREF)) resolve_symlink(lk-filename, max_path_len); - strcat(lk-filename, .lock); + strcat(lk-filename, LOCK_SUFFIX); lk-fd = open(lk-filename, O_RDWR | O_CREAT | O_EXCL, 0666); if (0 = lk-fd) { lk-owner = getpid(); @@ -314,7 +315,7 @@ int commit_lock_file(struct lock_file *lk) if (lk-fd = 0 close_lock_file(lk)) return -1; strcpy(result_file, lk-filename); - i = strlen(result_file) - 5; /* .lock */ + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */ Not a new bug since the previous code is broken too. Should probably checkstrlen(result_file) = 5 here before subtracting 5. Otherwise, a caller that calls commit_lock_file() with an already committed/closed lock_file can cause writing outside the bounds of the array on the line below. result_file[i] = 0; if (rename(lk-filename, result_file)) return -1; diff --git a/refs.c b/refs.c index 5ae8e69..828522d 100644 --- a/refs.c +++ b/refs.c @@ -74,7 +74,8 @@ out: if (refname[1] == '\0') return -1; /* Component equals .. */ } - if (cp - refname = 5 !memcmp(cp - 5, .lock, 5)) + if (cp - refname = LOCK_SUFFIX_LEN + !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN)) return -1; /* Refname ends with .lock. */ return cp - refname; } @@ -2545,11 +2546,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag) { if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) { /* loose */ - int err, i = strlen(lock-lk-filename) - 5; /* .lock */ + int err, i = strlen(lock-lk-filename) - LOCK_SUFFIX_LEN; lock-lk-filename[i] = 0; err = unlink_or_warn(lock-lk-filename); - lock-lk-filename[i] = '.'; + lock-lk-filename[i] = LOCK_SUFFIX[0]; if (err errno != ENOENT) return 1; } -- 2.1.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 -- 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 v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
Maybe we should not have a public constant defined for the length : +#define LOCK_SUFFIX_LEN 5 since it encourages unsafe code like : (this was unsafe long before your patch so not a regression) + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */ result_file[i] = 0; What about removing LOCK_SUFFIX_LEN from the public API and introduce a helper function something like : /* pointer to the character where the lock suffix starts */ char *lock_suffix_ptr_safe(const char *filename) { size_t len = strlen(filename); if (len 5) die(BUG:... if (strcmp(filename + len - 5, LOCK_SUFFIX) die(BUG:... return filename + len - 5; } and use it instead? On Sat, Sep 6, 2014 at 12:50 AM, Michael Haggerty mhag...@alum.mit.edu wrote: There are a few places that use these values, so define constants for them. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- cache.h| 4 lockfile.c | 11 ++- refs.c | 7 --- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/cache.h b/cache.h index da77094..41d829b 100644 --- a/cache.h +++ b/cache.h @@ -569,6 +569,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st); #define REFRESH_IN_PORCELAIN 0x0020 /* user friendly output, not needs update */ extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg); +/* String appended to a filename to derive the lockfile name: */ +#define LOCK_SUFFIX .lock +#define LOCK_SUFFIX_LEN 5 + struct lock_file { struct lock_file *next; int fd; diff --git a/lockfile.c b/lockfile.c index 964b3fc..bfea333 100644 --- a/lockfile.c +++ b/lockfile.c @@ -176,10 +176,11 @@ static char *resolve_symlink(char *p, size_t s) static int lock_file(struct lock_file *lk, const char *path, int flags) { /* -* subtract 5 from size to make sure there's room for adding -* .lock for the lock file name +* subtract LOCK_SUFFIX_LEN from size to make sure there's +* room for adding .lock for the lock file name: */ - static const size_t max_path_len = sizeof(lk-filename) - 5; + static const size_t max_path_len = sizeof(lk-filename) - + LOCK_SUFFIX_LEN; if (!lock_file_list) { /* One-time initialization */ @@ -204,7 +205,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) strcpy(lk-filename, path); if (!(flags LOCK_NODEREF)) resolve_symlink(lk-filename, max_path_len); - strcat(lk-filename, .lock); + strcat(lk-filename, LOCK_SUFFIX); lk-fd = open(lk-filename, O_RDWR | O_CREAT | O_EXCL, 0666); if (0 = lk-fd) { lk-owner = getpid(); @@ -314,7 +315,7 @@ int commit_lock_file(struct lock_file *lk) if (lk-fd = 0 close_lock_file(lk)) return -1; strcpy(result_file, lk-filename); - i = strlen(result_file) - 5; /* .lock */ + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */ result_file[i] = 0; if (rename(lk-filename, result_file)) return -1; diff --git a/refs.c b/refs.c index 5ae8e69..828522d 100644 --- a/refs.c +++ b/refs.c @@ -74,7 +74,8 @@ out: if (refname[1] == '\0') return -1; /* Component equals .. */ } - if (cp - refname = 5 !memcmp(cp - 5, .lock, 5)) + if (cp - refname = LOCK_SUFFIX_LEN + !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN)) return -1; /* Refname ends with .lock. */ return cp - refname; } @@ -2545,11 +2546,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag) { if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) { /* loose */ - int err, i = strlen(lock-lk-filename) - 5; /* .lock */ + int err, i = strlen(lock-lk-filename) - LOCK_SUFFIX_LEN; lock-lk-filename[i] = 0; err = unlink_or_warn(lock-lk-filename); - lock-lk-filename[i] = '.'; + lock-lk-filename[i] = LOCK_SUFFIX[0]; if (err errno != ENOENT) return 1; } -- 2.1.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 -- 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 v4 10/32] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
There are a few places that use these values, so define constants for them. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- cache.h| 4 lockfile.c | 11 ++- refs.c | 7 --- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/cache.h b/cache.h index da77094..41d829b 100644 --- a/cache.h +++ b/cache.h @@ -569,6 +569,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st); #define REFRESH_IN_PORCELAIN 0x0020 /* user friendly output, not needs update */ extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg); +/* String appended to a filename to derive the lockfile name: */ +#define LOCK_SUFFIX .lock +#define LOCK_SUFFIX_LEN 5 + struct lock_file { struct lock_file *next; int fd; diff --git a/lockfile.c b/lockfile.c index 964b3fc..bfea333 100644 --- a/lockfile.c +++ b/lockfile.c @@ -176,10 +176,11 @@ static char *resolve_symlink(char *p, size_t s) static int lock_file(struct lock_file *lk, const char *path, int flags) { /* -* subtract 5 from size to make sure there's room for adding -* .lock for the lock file name +* subtract LOCK_SUFFIX_LEN from size to make sure there's +* room for adding .lock for the lock file name: */ - static const size_t max_path_len = sizeof(lk-filename) - 5; + static const size_t max_path_len = sizeof(lk-filename) - + LOCK_SUFFIX_LEN; if (!lock_file_list) { /* One-time initialization */ @@ -204,7 +205,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) strcpy(lk-filename, path); if (!(flags LOCK_NODEREF)) resolve_symlink(lk-filename, max_path_len); - strcat(lk-filename, .lock); + strcat(lk-filename, LOCK_SUFFIX); lk-fd = open(lk-filename, O_RDWR | O_CREAT | O_EXCL, 0666); if (0 = lk-fd) { lk-owner = getpid(); @@ -314,7 +315,7 @@ int commit_lock_file(struct lock_file *lk) if (lk-fd = 0 close_lock_file(lk)) return -1; strcpy(result_file, lk-filename); - i = strlen(result_file) - 5; /* .lock */ + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */ result_file[i] = 0; if (rename(lk-filename, result_file)) return -1; diff --git a/refs.c b/refs.c index 5ae8e69..828522d 100644 --- a/refs.c +++ b/refs.c @@ -74,7 +74,8 @@ out: if (refname[1] == '\0') return -1; /* Component equals .. */ } - if (cp - refname = 5 !memcmp(cp - 5, .lock, 5)) + if (cp - refname = LOCK_SUFFIX_LEN + !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN)) return -1; /* Refname ends with .lock. */ return cp - refname; } @@ -2545,11 +2546,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag) { if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) { /* loose */ - int err, i = strlen(lock-lk-filename) - 5; /* .lock */ + int err, i = strlen(lock-lk-filename) - LOCK_SUFFIX_LEN; lock-lk-filename[i] = 0; err = unlink_or_warn(lock-lk-filename); - lock-lk-filename[i] = '.'; + lock-lk-filename[i] = LOCK_SUFFIX[0]; if (err errno != ENOENT) return 1; } -- 2.1.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