From b862b61c03797fd00490bb8caf05be840b79c6cb Mon Sep 17 00:00:00 2001 From: Matthieu Moy Date: Mon, 22 Feb 2010 23:32:13 +0100 Subject: git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument. gitmkstemps emulates the behavior of mkstemps, which is usually used to create files in a shared directory like /tmp/, hence, it creates files with permission 0600. Add git_mkstemps_mode() that allows us to specify the desired mode, and make git_mkstemps() a wrapper that always uses 0600 to call it. Later we will use git_mkstemps_mode() when creating pack files. Signed-off-by: Matthieu Moy Signed-off-by: Junio C Hamano --- wrapper.c | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'wrapper.c') diff --git a/wrapper.c b/wrapper.c index 0e3e20a3fd..673762fde9 100644 --- a/wrapper.c +++ b/wrapper.c @@ -204,6 +204,16 @@ int xmkstemp(char *template) return fd; } +int xmkstemp_mode(char *template, int mode) +{ + int fd; + + fd = git_mkstemp_mode(template, mode); + if (fd < 0) + die_errno("Unable to create temporary file"); + return fd; +} + /* * zlib wrappers to make sure we don't silently miss errors * at init time. -- cgit v1.3-5-g9baa From f80c7ae8fe9c0f3ce93c96a2dccaba34e456e33a Mon Sep 17 00:00:00 2001 From: Matthieu Moy Date: Mon, 22 Feb 2010 23:32:14 +0100 Subject: Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later. We used to create 0600 files, and then use chmod to set the group and other permission bits to the umask. This usually has the same effect as a normal file creation with a umask. But in the presence of ACLs, the group permission plays the role of the ACL mask: the "g" bits of newly created files are chosen according to default ACL mask of the directory, not according to the umask, and doing a chmod() on these "g" bits affect the ACL's mask instead of actual group permission. In other words, creating files with 0600 and then doing a chmod to the umask creates files which are unreadable by users allowed in the default ACL. To create the files without breaking ACLs, we let the umask do it's job at the file's creation time, and get rid of the later chmod. Signed-off-by: Matthieu Moy Signed-off-by: Junio C Hamano --- builtin-pack-objects.c | 18 ++---------------- t/t1304-default-acl.sh | 2 +- wrapper.c | 10 +++++++--- 3 files changed, 10 insertions(+), 20 deletions(-) (limited to 'wrapper.c') diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index e1d3adf405..539e75d56f 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -464,9 +464,6 @@ static int write_one(struct sha1file *f, return 1; } -/* forward declaration for write_pack_file */ -static int adjust_perm(const char *path, mode_t mode); - static void write_pack_file(void) { uint32_t i = 0, j; @@ -523,21 +520,17 @@ static void write_pack_file(void) } if (!pack_to_stdout) { - mode_t mode = umask(0); struct stat st; const char *idx_tmp_name; char tmpname[PATH_MAX]; - umask(mode); - mode = 0444 & ~mode; - idx_tmp_name = write_idx_file(NULL, written_list, nr_written, sha1); snprintf(tmpname, sizeof(tmpname), "%s-%s.pack", base_name, sha1_to_hex(sha1)); free_pack_by_name(tmpname); - if (adjust_perm(pack_tmp_name, mode)) + if (adjust_shared_perm(pack_tmp_name)) die_errno("unable to make temporary pack file readable"); if (rename(pack_tmp_name, tmpname)) die_errno("unable to rename temporary pack file"); @@ -565,7 +558,7 @@ static void write_pack_file(void) snprintf(tmpname, sizeof(tmpname), "%s-%s.idx", base_name, sha1_to_hex(sha1)); - if (adjust_perm(idx_tmp_name, mode)) + if (adjust_shared_perm(idx_tmp_name)) die_errno("unable to make temporary index file readable"); if (rename(idx_tmp_name, tmpname)) die_errno("unable to rename temporary index file"); @@ -2125,13 +2118,6 @@ static void get_object_list(int ac, const char **av) loosen_unused_packed_objects(&revs); } -static int adjust_perm(const char *path, mode_t mode) -{ - if (chmod(path, mode)) - return -1; - return adjust_shared_perm(path); -} - int cmd_pack_objects(int argc, const char **argv, const char *prefix) { int use_internal_rev_list = 0; diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh index 07dd6af99c..8472dbb44a 100755 --- a/t/t1304-default-acl.sh +++ b/t/t1304-default-acl.sh @@ -59,7 +59,7 @@ test_expect_failure 'Objects creation does not break ACLs with restrictive umask check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 ' -test_expect_failure 'git gc does not break ACLs with restrictive umask' ' +test_expect_success 'git gc does not break ACLs with restrictive umask' ' git gc && check_perms_and_acl .git/objects/pack/*.pack ' diff --git a/wrapper.c b/wrapper.c index 673762fde9..9c71b21242 100644 --- a/wrapper.c +++ b/wrapper.c @@ -277,10 +277,14 @@ int git_inflate(z_streamp strm, int flush) int odb_mkstemp(char *template, size_t limit, const char *pattern) { int fd; - + /* + * we let the umask do its job, don't try to be more + * restrictive except to remove write permission. + */ + int mode = 0444; snprintf(template, limit, "%s/%s", get_object_directory(), pattern); - fd = mkstemp(template); + fd = git_mkstemp_mode(template, mode); if (0 <= fd) return fd; @@ -289,7 +293,7 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern) snprintf(template, limit, "%s/%s", get_object_directory(), pattern); safe_create_leading_directories(template); - return xmkstemp(template); + return xmkstemp_mode(template, mode); } int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1) -- cgit v1.3-5-g9baa From a9a746364bd26d333c7229c6f7e851b507cd284a Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Wed, 24 Mar 2010 16:22:34 -0400 Subject: Make xmalloc and xrealloc thread-safe By providing a hook for the routine responsible for trying to free some memory on malloc failure, we can ensure that the called routine is protected by the appropriate locks when threads are in play. The obvious offender here was pack-objects which was calling xmalloc() within threads while release_pack_memory() is not thread safe. Signed-off-by: Nicolas Pitre Signed-off-by: Junio C Hamano --- builtin-pack-objects.c | 9 +++++++++ git-compat-util.h | 2 ++ wrapper.c | 20 ++++++++++++++++---- 3 files changed, 27 insertions(+), 4 deletions(-) (limited to 'wrapper.c') diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index 539e75d56f..0ecc198422 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -1549,6 +1549,13 @@ static void find_deltas(struct object_entry **list, unsigned *list_size, #ifndef NO_PTHREADS +static void try_to_free_from_threads(size_t size) +{ + read_lock(); + release_pack_memory(size, -1); + read_unlock(); +} + /* * The main thread waits on the condition that (at least) one of the workers * has stopped working (which is indicated in the .working member of @@ -1583,10 +1590,12 @@ static void init_threaded_search(void) pthread_mutex_init(&cache_mutex, NULL); pthread_mutex_init(&progress_mutex, NULL); pthread_cond_init(&progress_cond, NULL); + set_try_to_free_routine(try_to_free_from_threads); } static void cleanup_threaded_search(void) { + set_try_to_free_routine(NULL); pthread_cond_destroy(&progress_cond); pthread_mutex_destroy(&read_mutex); pthread_mutex_destroy(&cache_mutex); diff --git a/git-compat-util.h b/git-compat-util.h index a3c4537366..1c171db8b1 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -346,6 +346,8 @@ static inline char *gitstrchrnul(const char *s, int c) extern void release_pack_memory(size_t, int); +extern void set_try_to_free_routine(void (*routine)(size_t)); + extern char *xstrdup(const char *str); extern void *xmalloc(size_t size); extern void *xmallocz(size_t size); diff --git a/wrapper.c b/wrapper.c index 9c71b21242..62edb57338 100644 --- a/wrapper.c +++ b/wrapper.c @@ -3,11 +3,23 @@ */ #include "cache.h" +static void try_to_free_builtin(size_t size) +{ + release_pack_memory(size, -1); +} + +static void (*try_to_free_routine)(size_t size) = try_to_free_builtin; + +void set_try_to_free_routine(void (*routine)(size_t)) +{ + try_to_free_routine = (routine) ? routine : try_to_free_builtin; +} + char *xstrdup(const char *str) { char *ret = strdup(str); if (!ret) { - release_pack_memory(strlen(str) + 1, -1); + try_to_free_routine(strlen(str) + 1); ret = strdup(str); if (!ret) die("Out of memory, strdup failed"); @@ -21,7 +33,7 @@ void *xmalloc(size_t size) if (!ret && !size) ret = malloc(1); if (!ret) { - release_pack_memory(size, -1); + try_to_free_routine(size); ret = malloc(size); if (!ret && !size) ret = malloc(1); @@ -67,7 +79,7 @@ void *xrealloc(void *ptr, size_t size) if (!ret && !size) ret = realloc(ptr, 1); if (!ret) { - release_pack_memory(size, -1); + try_to_free_routine(size); ret = realloc(ptr, size); if (!ret && !size) ret = realloc(ptr, 1); @@ -83,7 +95,7 @@ void *xcalloc(size_t nmemb, size_t size) if (!ret && (!nmemb || !size)) ret = calloc(1, 1); if (!ret) { - release_pack_memory(nmemb * size, -1); + try_to_free_routine(nmemb * size); ret = calloc(nmemb, size); if (!ret && (!nmemb || !size)) ret = calloc(1, 1); -- cgit v1.3-5-g9baa From 10e13ec8ed36019d131d27cd9fe2e8cc0f99b896 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Fri, 26 Mar 2010 15:25:32 +0000 Subject: Generalise the unlink_or_warn function This patch moves the warning code of the unlink_or_warn function into a separate function named warn_if_unremovable so that it may be reused. Signed-off-by: Peter Collingbourne Signed-off-by: Junio C Hamano --- wrapper.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) (limited to 'wrapper.c') diff --git a/wrapper.c b/wrapper.c index 9c71b21242..0bbff56e2c 100644 --- a/wrapper.c +++ b/wrapper.c @@ -311,18 +311,20 @@ int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1) return open(name, O_RDWR|O_CREAT|O_EXCL, 0600); } -int unlink_or_warn(const char *file) +static int warn_if_unremovable(const char *op, const char *file, int rc) { - int rc = unlink(file); - if (rc < 0) { int err = errno; if (ENOENT != err) { - warning("unable to unlink %s: %s", - file, strerror(errno)); + warning("unable to %s %s: %s", + op, file, strerror(errno)); errno = err; } } return rc; } +int unlink_or_warn(const char *file) +{ + return warn_if_unremovable("unlink", file, unlink(file)); +} -- cgit v1.3-5-g9baa From d1723296af67e6bbadf6e73cd1e921aefafe491f Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Fri, 26 Mar 2010 15:25:33 +0000 Subject: Implement the rmdir_or_warn function This patch implements an rmdir_or_warn function (like unlink_or_warn but for directories) that uses the generalised warning code in warn_if_unremovable. Signed-off-by: Peter Collingbourne Signed-off-by: Junio C Hamano --- git-compat-util.h | 4 ++++ wrapper.c | 5 +++++ 2 files changed, 9 insertions(+) (limited to 'wrapper.c') diff --git a/git-compat-util.h b/git-compat-util.h index a3c4537366..67ea4c89f5 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -469,5 +469,9 @@ void git_qsort(void *base, size_t nmemb, size_t size, * Always returns the return value of unlink(2). */ int unlink_or_warn(const char *path); +/* + * Likewise for rmdir(2). + */ +int rmdir_or_warn(const char *path); #endif diff --git a/wrapper.c b/wrapper.c index 0bbff56e2c..4017bff6bb 100644 --- a/wrapper.c +++ b/wrapper.c @@ -328,3 +328,8 @@ int unlink_or_warn(const char *file) { return warn_if_unremovable("unlink", file, unlink(file)); } + +int rmdir_or_warn(const char *file) +{ + return warn_if_unremovable("rmdir", file, rmdir(file)); +} -- cgit v1.3-5-g9baa From 80d706afed6c6c6fb3ac9c168a6a958244405b45 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Fri, 26 Mar 2010 15:25:34 +0000 Subject: Introduce remove_or_warn function This patch introduces the remove_or_warn function which is a generalised version of the {unlink,rmdir}_or_warn functions. It takes an additional parameter indicating the mode of the file to be removed. The patch also modifies certain functions to use remove_or_warn where appropriate, and adds a test case for a bug fixed by the use of remove_or_warn. Signed-off-by: Peter Collingbourne Signed-off-by: Junio C Hamano --- builtin/apply.c | 6 +----- git-compat-util.h | 5 +++++ t/t4134-apply-submodule.sh | 38 ++++++++++++++++++++++++++++++++++++++ unpack-trees.c | 12 ++---------- wrapper.c | 5 +++++ 5 files changed, 51 insertions(+), 15 deletions(-) create mode 100755 t/t4134-apply-submodule.sh (limited to 'wrapper.c') diff --git a/builtin/apply.c b/builtin/apply.c index 7ca90472c1..65a594c985 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3144,11 +3144,7 @@ static void remove_file(struct patch *patch, int rmdir_empty) die("unable to remove %s from index", patch->old_name); } if (!cached) { - if (S_ISGITLINK(patch->old_mode)) { - if (rmdir(patch->old_name)) - warning("unable to remove submodule %s", - patch->old_name); - } else if (!unlink_or_warn(patch->old_name) && rmdir_empty) { + if (!remove_or_warn(patch->old_mode, patch->old_name) && rmdir_empty) { remove_path(patch->old_name); } } diff --git a/git-compat-util.h b/git-compat-util.h index 67ea4c89f5..3ebf96690a 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -473,5 +473,10 @@ int unlink_or_warn(const char *path); * Likewise for rmdir(2). */ int rmdir_or_warn(const char *path); +/* + * Calls the correct function out of {unlink,rmdir}_or_warn based on + * the supplied file mode. + */ +int remove_or_warn(unsigned int mode, const char *path); #endif diff --git a/t/t4134-apply-submodule.sh b/t/t4134-apply-submodule.sh new file mode 100755 index 0000000000..1b82f93cff --- /dev/null +++ b/t/t4134-apply-submodule.sh @@ -0,0 +1,38 @@ +#!/bin/sh +# +# Copyright (c) 2010 Peter Collingbourne +# + +test_description='git apply submodule tests' + +. ./test-lib.sh + +test_expect_success setup ' + cat > create-sm.patch < remove-sm.patch <name, ce_namelen(ce))) return; - if (S_ISGITLINK(ce->ce_mode)) { - if (rmdir(ce->name)) { - warning("unable to rmdir %s: %s", - ce->name, strerror(errno)); - return; - } - } - else - if (unlink_or_warn(ce->name)) - return; + if (remove_or_warn(ce->ce_mode, ce->name)) + return; schedule_dir_for_removal(ce->name, ce_namelen(ce)); } diff --git a/wrapper.c b/wrapper.c index 4017bff6bb..10a6750795 100644 --- a/wrapper.c +++ b/wrapper.c @@ -333,3 +333,8 @@ int rmdir_or_warn(const char *file) { return warn_if_unremovable("rmdir", file, rmdir(file)); } + +int remove_or_warn(unsigned int mode, const char *file) +{ + return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file); +} -- cgit v1.3-5-g9baa From 851c34b04e0ce866e15c28e144986eca7533a6f4 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 8 May 2010 17:13:49 +0200 Subject: Have set_try_to_free_routine return the previous routine This effectively requires from the callers of set_try_to_free_routine to treat the try-to-free-routines as a stack. We will need this for the next patch where the only current caller cannot depend on that the previously set routine was the default routine. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- builtin-pack-objects.c | 6 ++++-- git-compat-util.h | 3 ++- wrapper.c | 6 ++++-- 3 files changed, 10 insertions(+), 5 deletions(-) (limited to 'wrapper.c') diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index 26fc7cd5a6..5279cd99f2 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -1556,6 +1556,8 @@ static void try_to_free_from_threads(size_t size) read_unlock(); } +try_to_free_t old_try_to_free_routine; + /* * The main thread waits on the condition that (at least) one of the workers * has stopped working (which is indicated in the .working member of @@ -1590,12 +1592,12 @@ static void init_threaded_search(void) pthread_mutex_init(&cache_mutex, NULL); pthread_mutex_init(&progress_mutex, NULL); pthread_cond_init(&progress_cond, NULL); - set_try_to_free_routine(try_to_free_from_threads); + old_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads); } static void cleanup_threaded_search(void) { - set_try_to_free_routine(NULL); + set_try_to_free_routine(old_try_to_free_routine); pthread_cond_destroy(&progress_cond); pthread_mutex_destroy(&read_mutex); pthread_mutex_destroy(&cache_mutex); diff --git a/git-compat-util.h b/git-compat-util.h index 1c171db8b1..828aadaf3a 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -346,7 +346,8 @@ static inline char *gitstrchrnul(const char *s, int c) extern void release_pack_memory(size_t, int); -extern void set_try_to_free_routine(void (*routine)(size_t)); +typedef void (*try_to_free_t)(size_t); +extern try_to_free_t set_try_to_free_routine(try_to_free_t); extern char *xstrdup(const char *str); extern void *xmalloc(size_t size); diff --git a/wrapper.c b/wrapper.c index 62edb57338..8aa9df9b83 100644 --- a/wrapper.c +++ b/wrapper.c @@ -10,9 +10,11 @@ static void try_to_free_builtin(size_t size) static void (*try_to_free_routine)(size_t size) = try_to_free_builtin; -void set_try_to_free_routine(void (*routine)(size_t)) +try_to_free_t set_try_to_free_routine(try_to_free_t routine) { - try_to_free_routine = (routine) ? routine : try_to_free_builtin; + try_to_free_t old = try_to_free_routine; + try_to_free_routine = routine; + return old; } char *xstrdup(const char *str) -- cgit v1.3-5-g9baa From 8bd9fd50014424643bddfde51423d42e0fc60800 Mon Sep 17 00:00:00 2001 From: Matthieu Moy Date: Fri, 20 Aug 2010 17:09:51 +0200 Subject: xmalloc: include size in the failure message Out-of-memory errors can either be actual lack of memory, or bugs (like code trying to call xmalloc(-1) by mistake). A little more information may help tracking bugs reported by users. Signed-off-by: Matthieu Moy Signed-off-by: Junio C Hamano --- wrapper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'wrapper.c') diff --git a/wrapper.c b/wrapper.c index afb4f6f773..fd8ead33ed 100644 --- a/wrapper.c +++ b/wrapper.c @@ -40,7 +40,8 @@ void *xmalloc(size_t size) if (!ret && !size) ret = malloc(1); if (!ret) - die("Out of memory, malloc failed"); + die("Out of memory, malloc failed (tried to allocate %lu bytes)", + (unsigned long)size); } #ifdef XMALLOC_POISON memset(ret, 0xA5, size); -- cgit v1.3-5-g9baa From 58ecbd5edeb2357c313db75bc49d45981a2061b7 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 6 Nov 2010 06:44:11 -0500 Subject: wrapper: move xmmap() to sha1_file.c wrapper.o depends on sha1_file.o for a number of reasons. One is release_pack_memory(). xmmap function calls mmap, discarding unused pack windows when necessary to relieve memory pressure. Simple git programs using wrapper.o as a friendly libc do not need this functionality. So move xmmap to sha1_file.o, where release_pack_memory() is. Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- sha1_file.c | 15 +++++++++++++++ wrapper.c | 15 --------------- 2 files changed, 15 insertions(+), 15 deletions(-) (limited to 'wrapper.c') diff --git a/sha1_file.c b/sha1_file.c index 0cd9435619..8e299ae85c 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -576,6 +576,21 @@ void release_pack_memory(size_t need, int fd) ; /* nothing */ } +void *xmmap(void *start, size_t length, + int prot, int flags, int fd, off_t offset) +{ + void *ret = mmap(start, length, prot, flags, fd, offset); + if (ret == MAP_FAILED) { + if (!length) + return NULL; + release_pack_memory(length, fd); + ret = mmap(start, length, prot, flags, fd, offset); + if (ret == MAP_FAILED) + die_errno("Out of memory? mmap failed"); + } + return ret; +} + void close_pack_windows(struct packed_git *p) { while (p->windows) { diff --git a/wrapper.c b/wrapper.c index fd8ead33ed..3195ef32b7 100644 --- a/wrapper.c +++ b/wrapper.c @@ -108,21 +108,6 @@ void *xcalloc(size_t nmemb, size_t size) return ret; } -void *xmmap(void *start, size_t length, - int prot, int flags, int fd, off_t offset) -{ - void *ret = mmap(start, length, prot, flags, fd, offset); - if (ret == MAP_FAILED) { - if (!length) - return NULL; - release_pack_memory(length, fd); - ret = mmap(start, length, prot, flags, fd, offset); - if (ret == MAP_FAILED) - die_errno("Out of memory? mmap failed"); - } - return ret; -} - /* * xread() is the same a read(), but it automatically restarts read() * operations with a recoverable error (EAGAIN and EINTR). xread() -- cgit v1.3-5-g9baa From 463db9b104b5db7d574ce4c5ede8caaa6d02ff4c Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 6 Nov 2010 06:45:38 -0500 Subject: wrapper: move odb_* to environment.c The odb_mkstemp and odb_pack_keep functions open files under the $GIT_OBJECT_DIRECTORY directory. This requires access to the git configuration which very simple programs do not need. Move these functions to environment.o, closer to their dependencies. This should make it easier for programs to link to wrapper.o without linking to environment.o. Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- environment.c | 37 +++++++++++++++++++++++++++++++++++++ git-compat-util.h | 1 + wrapper.c | 37 ------------------------------------- 3 files changed, 38 insertions(+), 37 deletions(-) (limited to 'wrapper.c') diff --git a/environment.c b/environment.c index de5581fe51..95777f4c7f 100644 --- a/environment.c +++ b/environment.c @@ -171,6 +171,43 @@ char *get_object_directory(void) return git_object_dir; } +int odb_mkstemp(char *template, size_t limit, const char *pattern) +{ + int fd; + /* + * we let the umask do its job, don't try to be more + * restrictive except to remove write permission. + */ + int mode = 0444; + snprintf(template, limit, "%s/%s", + get_object_directory(), pattern); + fd = git_mkstemp_mode(template, mode); + if (0 <= fd) + return fd; + + /* slow path */ + /* some mkstemp implementations erase template on failure */ + snprintf(template, limit, "%s/%s", + get_object_directory(), pattern); + safe_create_leading_directories(template); + return xmkstemp_mode(template, mode); +} + +int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1) +{ + int fd; + + snprintf(name, namesz, "%s/pack/pack-%s.keep", + get_object_directory(), sha1_to_hex(sha1)); + fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600); + if (0 <= fd) + return fd; + + /* slow path */ + safe_create_leading_directories(name); + return open(name, O_RDWR|O_CREAT|O_EXCL, 0600); +} + char *get_index_file(void) { if (!git_index_file) diff --git a/git-compat-util.h b/git-compat-util.h index 2af8d3edbe..029162eccc 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -404,6 +404,7 @@ extern ssize_t xwrite(int fd, const void *buf, size_t len); extern int xdup(int fd); extern FILE *xfdopen(int fd, const char *mode); extern int xmkstemp(char *template); +extern int xmkstemp_mode(char *template, int mode); extern int odb_mkstemp(char *template, size_t limit, const char *pattern); extern int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1); diff --git a/wrapper.c b/wrapper.c index 3195ef32b7..e67b70a108 100644 --- a/wrapper.c +++ b/wrapper.c @@ -274,43 +274,6 @@ int git_inflate(z_streamp strm, int flush) return ret; } -int odb_mkstemp(char *template, size_t limit, const char *pattern) -{ - int fd; - /* - * we let the umask do its job, don't try to be more - * restrictive except to remove write permission. - */ - int mode = 0444; - snprintf(template, limit, "%s/%s", - get_object_directory(), pattern); - fd = git_mkstemp_mode(template, mode); - if (0 <= fd) - return fd; - - /* slow path */ - /* some mkstemp implementations erase template on failure */ - snprintf(template, limit, "%s/%s", - get_object_directory(), pattern); - safe_create_leading_directories(template); - return xmkstemp_mode(template, mode); -} - -int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1) -{ - int fd; - - snprintf(name, namesz, "%s/pack/pack-%s.keep", - get_object_directory(), sha1_to_hex(sha1)); - fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600); - if (0 <= fd) - return fd; - - /* slow path */ - safe_create_leading_directories(name); - return open(name, O_RDWR|O_CREAT|O_EXCL, 0600); -} - static int warn_if_unremovable(const char *op, const char *file, int rc) { if (rc < 0) { -- cgit v1.3-5-g9baa From 33f239365cce2682a1faac0d5670d684aa1981ad Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 6 Nov 2010 06:46:31 -0500 Subject: path helpers: move git_mkstemp* to wrapper.c git_mkstemp_mode and related functions do not require access to specialized git machinery, unlike some other functions from path.c (like set_shared_perm()). Move them to wrapper.c where the wrapper xmkstemp_mode is defined. This eliminates a dependency of wrapper.o on environment.o via path.o. With typical linkers (e.g., gcc), that dependency makes programs that use functions from wrapper.o and not environment.o or path.o larger than they need to be. Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- path.c | 113 -------------------------------------------------------------- wrapper.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 113 deletions(-) (limited to 'wrapper.c') diff --git a/path.c b/path.c index a2c9d1e24a..8951333cb8 100644 --- a/path.c +++ b/path.c @@ -161,119 +161,6 @@ char *git_path_submodule(const char *path, const char *fmt, ...) return cleanup_path(pathname); } -/* git_mkstemp() - create tmp file honoring TMPDIR variable */ -int git_mkstemp(char *path, size_t len, const char *template) -{ - const char *tmp; - size_t n; - - tmp = getenv("TMPDIR"); - if (!tmp) - tmp = "/tmp"; - n = snprintf(path, len, "%s/%s", tmp, template); - if (len <= n) { - errno = ENAMETOOLONG; - return -1; - } - return mkstemp(path); -} - -/* git_mkstemps() - create tmp file with suffix honoring TMPDIR variable. */ -int git_mkstemps(char *path, size_t len, const char *template, int suffix_len) -{ - const char *tmp; - size_t n; - - tmp = getenv("TMPDIR"); - if (!tmp) - tmp = "/tmp"; - n = snprintf(path, len, "%s/%s", tmp, template); - if (len <= n) { - errno = ENAMETOOLONG; - return -1; - } - return mkstemps(path, suffix_len); -} - -/* Adapted from libiberty's mkstemp.c. */ - -#undef TMP_MAX -#define TMP_MAX 16384 - -int git_mkstemps_mode(char *pattern, int suffix_len, int mode) -{ - static const char letters[] = - "abcdefghijklmnopqrstuvwxyz" - "ABCDEFGHIJKLMNOPQRSTUVWXYZ" - "0123456789"; - static const int num_letters = 62; - uint64_t value; - struct timeval tv; - char *template; - size_t len; - int fd, count; - - len = strlen(pattern); - - if (len < 6 + suffix_len) { - errno = EINVAL; - return -1; - } - - if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) { - errno = EINVAL; - return -1; - } - - /* - * Replace pattern's XXXXXX characters with randomness. - * Try TMP_MAX different filenames. - */ - gettimeofday(&tv, NULL); - value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid(); - template = &pattern[len - 6 - suffix_len]; - for (count = 0; count < TMP_MAX; ++count) { - uint64_t v = value; - /* Fill in the random bits. */ - template[0] = letters[v % num_letters]; v /= num_letters; - template[1] = letters[v % num_letters]; v /= num_letters; - template[2] = letters[v % num_letters]; v /= num_letters; - template[3] = letters[v % num_letters]; v /= num_letters; - template[4] = letters[v % num_letters]; v /= num_letters; - template[5] = letters[v % num_letters]; v /= num_letters; - - fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); - if (fd > 0) - return fd; - /* - * Fatal error (EPERM, ENOSPC etc). - * It doesn't make sense to loop. - */ - if (errno != EEXIST) - break; - /* - * This is a random value. It is only necessary that - * the next TMP_MAX values generated by adding 7777 to - * VALUE are different with (module 2^32). - */ - value += 7777; - } - /* We return the null string if we can't find a unique file name. */ - pattern[0] = '\0'; - return -1; -} - -int git_mkstemp_mode(char *pattern, int mode) -{ - /* mkstemp is just mkstemps with no suffix */ - return git_mkstemps_mode(pattern, 0, mode); -} - -int gitmkstemps(char *pattern, int suffix_len) -{ - return git_mkstemps_mode(pattern, suffix_len, 0600); -} - int validate_headref(const char *path) { struct stat st; diff --git a/wrapper.c b/wrapper.c index e67b70a108..b3efefb539 100644 --- a/wrapper.c +++ b/wrapper.c @@ -204,6 +204,119 @@ int xmkstemp(char *template) return fd; } +/* git_mkstemp() - create tmp file honoring TMPDIR variable */ +int git_mkstemp(char *path, size_t len, const char *template) +{ + const char *tmp; + size_t n; + + tmp = getenv("TMPDIR"); + if (!tmp) + tmp = "/tmp"; + n = snprintf(path, len, "%s/%s", tmp, template); + if (len <= n) { + errno = ENAMETOOLONG; + return -1; + } + return mkstemp(path); +} + +/* git_mkstemps() - create tmp file with suffix honoring TMPDIR variable. */ +int git_mkstemps(char *path, size_t len, const char *template, int suffix_len) +{ + const char *tmp; + size_t n; + + tmp = getenv("TMPDIR"); + if (!tmp) + tmp = "/tmp"; + n = snprintf(path, len, "%s/%s", tmp, template); + if (len <= n) { + errno = ENAMETOOLONG; + return -1; + } + return mkstemps(path, suffix_len); +} + +/* Adapted from libiberty's mkstemp.c. */ + +#undef TMP_MAX +#define TMP_MAX 16384 + +int git_mkstemps_mode(char *pattern, int suffix_len, int mode) +{ + static const char letters[] = + "abcdefghijklmnopqrstuvwxyz" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + "0123456789"; + static const int num_letters = 62; + uint64_t value; + struct timeval tv; + char *template; + size_t len; + int fd, count; + + len = strlen(pattern); + + if (len < 6 + suffix_len) { + errno = EINVAL; + return -1; + } + + if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) { + errno = EINVAL; + return -1; + } + + /* + * Replace pattern's XXXXXX characters with randomness. + * Try TMP_MAX different filenames. + */ + gettimeofday(&tv, NULL); + value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid(); + template = &pattern[len - 6 - suffix_len]; + for (count = 0; count < TMP_MAX; ++count) { + uint64_t v = value; + /* Fill in the random bits. */ + template[0] = letters[v % num_letters]; v /= num_letters; + template[1] = letters[v % num_letters]; v /= num_letters; + template[2] = letters[v % num_letters]; v /= num_letters; + template[3] = letters[v % num_letters]; v /= num_letters; + template[4] = letters[v % num_letters]; v /= num_letters; + template[5] = letters[v % num_letters]; v /= num_letters; + + fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); + if (fd > 0) + return fd; + /* + * Fatal error (EPERM, ENOSPC etc). + * It doesn't make sense to loop. + */ + if (errno != EEXIST) + break; + /* + * This is a random value. It is only necessary that + * the next TMP_MAX values generated by adding 7777 to + * VALUE are different with (module 2^32). + */ + value += 7777; + } + /* We return the null string if we can't find a unique file name. */ + pattern[0] = '\0'; + return -1; +} + +int git_mkstemp_mode(char *pattern, int mode) +{ + /* mkstemp is just mkstemps with no suffix */ + return git_mkstemps_mode(pattern, 0, mode); +} + +int gitmkstemps(char *pattern, int suffix_len) +{ + return git_mkstemps_mode(pattern, suffix_len, 0600); +} + int xmkstemp_mode(char *template, int mode) { int fd; -- cgit v1.3-5-g9baa From b0613ce0f9ef3fd111f8c75b84ddd12f9f04fc87 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 6 Nov 2010 06:47:34 -0500 Subject: wrapper: give zlib wrappers their own translation unit Programs using xmalloc() but not git_inflate() require -lz on the linker command line because git_inflate() is in the same translation unit as xmalloc(). Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- Makefile | 1 + wrapper.c | 60 ------------------------------------------------------------ zlib.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 60 deletions(-) create mode 100644 zlib.c (limited to 'wrapper.c') diff --git a/Makefile b/Makefile index 1f1ce04edf..a8ba33635d 100644 --- a/Makefile +++ b/Makefile @@ -662,6 +662,7 @@ LIB_OBJS += write_or_die.o LIB_OBJS += ws.o LIB_OBJS += wt-status.o LIB_OBJS += xdiff-interface.o +LIB_OBJS += zlib.o BUILTIN_OBJS += builtin/add.o BUILTIN_OBJS += builtin/annotate.o diff --git a/wrapper.c b/wrapper.c index b3efefb539..185dfbcc46 100644 --- a/wrapper.c +++ b/wrapper.c @@ -327,66 +327,6 @@ int xmkstemp_mode(char *template, int mode) return fd; } -/* - * zlib wrappers to make sure we don't silently miss errors - * at init time. - */ -void git_inflate_init(z_streamp strm) -{ - const char *err; - - switch (inflateInit(strm)) { - case Z_OK: - return; - - case Z_MEM_ERROR: - err = "out of memory"; - break; - case Z_VERSION_ERROR: - err = "wrong version"; - break; - default: - err = "error"; - } - die("inflateInit: %s (%s)", err, strm->msg ? strm->msg : "no message"); -} - -void git_inflate_end(z_streamp strm) -{ - if (inflateEnd(strm) != Z_OK) - error("inflateEnd: %s", strm->msg ? strm->msg : "failed"); -} - -int git_inflate(z_streamp strm, int flush) -{ - int ret = inflate(strm, flush); - const char *err; - - switch (ret) { - /* Out of memory is fatal. */ - case Z_MEM_ERROR: - die("inflate: out of memory"); - - /* Data corruption errors: we may want to recover from them (fsck) */ - case Z_NEED_DICT: - err = "needs dictionary"; break; - case Z_DATA_ERROR: - err = "data stream error"; break; - case Z_STREAM_ERROR: - err = "stream consistency error"; break; - default: - err = "unknown error"; break; - - /* Z_BUF_ERROR: normal, needs more space in the output buffer */ - case Z_BUF_ERROR: - case Z_OK: - case Z_STREAM_END: - return ret; - } - error("inflate: %s (%s)", err, strm->msg ? strm->msg : "no message"); - return ret; -} - static int warn_if_unremovable(const char *op, const char *file, int rc) { if (rc < 0) { diff --git a/zlib.c b/zlib.c new file mode 100644 index 0000000000..c4d58da4e9 --- /dev/null +++ b/zlib.c @@ -0,0 +1,61 @@ +/* + * zlib wrappers to make sure we don't silently miss errors + * at init time. + */ +#include "cache.h" + +void git_inflate_init(z_streamp strm) +{ + const char *err; + + switch (inflateInit(strm)) { + case Z_OK: + return; + + case Z_MEM_ERROR: + err = "out of memory"; + break; + case Z_VERSION_ERROR: + err = "wrong version"; + break; + default: + err = "error"; + } + die("inflateInit: %s (%s)", err, strm->msg ? strm->msg : "no message"); +} + +void git_inflate_end(z_streamp strm) +{ + if (inflateEnd(strm) != Z_OK) + error("inflateEnd: %s", strm->msg ? strm->msg : "failed"); +} + +int git_inflate(z_streamp strm, int flush) +{ + int ret = inflate(strm, flush); + const char *err; + + switch (ret) { + /* Out of memory is fatal. */ + case Z_MEM_ERROR: + die("inflate: out of memory"); + + /* Data corruption errors: we may want to recover from them (fsck) */ + case Z_NEED_DICT: + err = "needs dictionary"; break; + case Z_DATA_ERROR: + err = "data stream error"; break; + case Z_STREAM_ERROR: + err = "stream consistency error"; break; + default: + err = "unknown error"; break; + + /* Z_BUF_ERROR: normal, needs more space in the output buffer */ + case Z_BUF_ERROR: + case Z_OK: + case Z_STREAM_END: + return ret; + } + error("inflate: %s (%s)", err, strm->msg ? strm->msg : "no message"); + return ret; +} -- cgit v1.3-5-g9baa From e0500293852910c2f44fce61e7eb856adbc20d8a Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 6 Nov 2010 14:00:38 -0500 Subject: Remove pack file handling dependency from wrapper.o MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As v1.7.0-rc0~43 (slim down "git show-index", 2010-01-21) explains, use of xmalloc() brings in a dependency on zlib, the sha1 lib, and the rest of git's object file access machinery via try_to_free_pack_memory. That is overkill when xmalloc is just being used as a convenience wrapper to exit when no memory is available. So defer setting try_to_free_pack_memory as try_to_free_routine until the first packfile is opened in add_packed_git(). After this change, a simple program using xmalloc() and no other functions will not pull in any code from libgit.a aside from wrapper.o and usage.o. Improved-by: René Scharfe Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- sha1_file.c | 11 +++++++++++ wrapper.c | 5 ++--- 2 files changed, 13 insertions(+), 3 deletions(-) (limited to 'wrapper.c') diff --git a/sha1_file.c b/sha1_file.c index 8e299ae85c..e0d2496bcd 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -818,11 +818,22 @@ static struct packed_git *alloc_packed_git(int extra) return p; } +static void try_to_free_pack_memory(size_t size) +{ + release_pack_memory(size, -1); +} + struct packed_git *add_packed_git(const char *path, int path_len, int local) { + static int have_set_try_to_free_routine; struct stat st; struct packed_git *p = alloc_packed_git(path_len + 2); + if (!have_set_try_to_free_routine) { + have_set_try_to_free_routine = 1; + set_try_to_free_routine(try_to_free_pack_memory); + } + /* * Make sure a corresponding .pack file exists and that * the index looks sane. diff --git a/wrapper.c b/wrapper.c index 185dfbcc46..4c1639f153 100644 --- a/wrapper.c +++ b/wrapper.c @@ -3,12 +3,11 @@ */ #include "cache.h" -static void try_to_free_builtin(size_t size) +static void do_nothing(size_t size) { - release_pack_memory(size, -1); } -static void (*try_to_free_routine)(size_t size) = try_to_free_builtin; +static void (*try_to_free_routine)(size_t size) = do_nothing; try_to_free_t set_try_to_free_routine(try_to_free_t routine) { -- cgit v1.3-5-g9baa From 00b0d7f77b65ed2d059f893bbd2011ba5bb4252d Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 21 Dec 2010 09:24:18 -0800 Subject: set_try_to_free_routine(NULL) means "do nothing special" This way, the next caller that wants to disable our memory reclamation machinery does not have to define its own do_nothing() stub. Signed-off-by: Junio C Hamano --- trace.c | 8 ++------ wrapper.c | 2 ++ 2 files changed, 4 insertions(+), 6 deletions(-) (limited to 'wrapper.c') diff --git a/trace.c b/trace.c index 62586fa371..0fb2a2c64b 100644 --- a/trace.c +++ b/trace.c @@ -25,10 +25,6 @@ #include "cache.h" #include "quote.h" -static void do_nothing(size_t unused) -{ -} - /* Get a trace file descriptor from GIT_TRACE env variable. */ static int get_trace_fd(int *need_close) { @@ -76,7 +72,7 @@ void trace_printf(const char *fmt, ...) if (!fd) return; - set_try_to_free_routine(do_nothing); /* is never reset */ + set_try_to_free_routine(NULL); /* is never reset */ strbuf_init(&buf, 64); va_start(ap, fmt); len = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap); @@ -108,7 +104,7 @@ void trace_argv_printf(const char **argv, const char *fmt, ...) if (!fd) return; - set_try_to_free_routine(do_nothing); /* is never reset */ + set_try_to_free_routine(NULL); /* is never reset */ strbuf_init(&buf, 64); va_start(ap, fmt); len = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap); diff --git a/wrapper.c b/wrapper.c index 4c1639f153..8d7dd31c4b 100644 --- a/wrapper.c +++ b/wrapper.c @@ -12,6 +12,8 @@ static void (*try_to_free_routine)(size_t size) = do_nothing; try_to_free_t set_try_to_free_routine(try_to_free_t routine) { try_to_free_t old = try_to_free_routine; + if (!routine) + routine = do_nothing; try_to_free_routine = routine; return old; } -- cgit v1.3-5-g9baa From 6cf6bb3e47ac2f667fa0b27a4222e903ff6fb77c Mon Sep 17 00:00:00 2001 From: Arnout Engelen Date: Sat, 18 Dec 2010 22:28:00 +0100 Subject: Improve error messages when temporary file creation fails Before, when creating a temporary file failed, a generic 'Unable to create temporary file' message was printed. In some cases this could lead to confusion as to which directory should be checked for correct permissions etc. This patch adds the template for the temporary filename to the error message, converting it to an absolute path if needed. A test verifies that the template is indeed printed when pointing to a nonexistent or unwritable directory. A copy of the original template is made in case mkstemp clears the template. Signed-off-by: Arnout Engelen Signed-off-by: Junio C Hamano --- Makefile | 1 + t/t0070-fundamental.sh | 13 +++++++++++++ test-mktemp.c | 14 ++++++++++++++ wrapper.c | 32 ++++++++++++++++++++++++++++---- 4 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 test-mktemp.c (limited to 'wrapper.c') diff --git a/Makefile b/Makefile index 57d9c65e03..03a51cb8f2 100644 --- a/Makefile +++ b/Makefile @@ -434,6 +434,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool TEST_PROGRAMS_NEED_X += test-svn-fe TEST_PROGRAMS_NEED_X += test-treap TEST_PROGRAMS_NEED_X += test-index-version +TEST_PROGRAMS_NEED_X += test-mktemp TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X)) diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 680d7d6861..9bee8bfd2e 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -12,4 +12,17 @@ test_expect_success 'character classes (isspace, isalpha etc.)' ' test-ctype ' +test_expect_success 'mktemp to nonexistent directory prints filename' ' + test_must_fail test-mktemp doesnotexist/testXXXXXX 2>err && + grep "doesnotexist/test" err +' + +test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' ' + mkdir cannotwrite && + chmod -w cannotwrite && + test_when_finished "chmod +w cannotwrite" && + test_must_fail test-mktemp cannotwrite/testXXXXXX 2>err && + grep "cannotwrite/test" err +' + test_done diff --git a/test-mktemp.c b/test-mktemp.c new file mode 100644 index 0000000000..c8c54213a3 --- /dev/null +++ b/test-mktemp.c @@ -0,0 +1,14 @@ +/* + * test-mktemp.c: code to exercise the creation of temporary files + */ +#include "git-compat-util.h" + +int main(int argc, char *argv[]) +{ + if (argc != 2) + usage("Expected 1 parameter defining the temporary file template"); + + xmkstemp(xstrdup(argv[1])); + + return 0; +} diff --git a/wrapper.c b/wrapper.c index 8d7dd31c4b..55b074ec46 100644 --- a/wrapper.c +++ b/wrapper.c @@ -198,10 +198,22 @@ FILE *xfdopen(int fd, const char *mode) int xmkstemp(char *template) { int fd; + char origtemplate[PATH_MAX]; + strlcpy(origtemplate, template, sizeof(origtemplate)); fd = mkstemp(template); - if (fd < 0) - die_errno("Unable to create temporary file"); + if (fd < 0) { + int saved_errno = errno; + const char *nonrelative_template; + + if (!template[0]) + template = origtemplate; + + nonrelative_template = make_nonrelative_path(template); + errno = saved_errno; + die_errno("Unable to create temporary file '%s'", + nonrelative_template); + } return fd; } @@ -321,10 +333,22 @@ int gitmkstemps(char *pattern, int suffix_len) int xmkstemp_mode(char *template, int mode) { int fd; + char origtemplate[PATH_MAX]; + strlcpy(origtemplate, template, sizeof(origtemplate)); fd = git_mkstemp_mode(template, mode); - if (fd < 0) - die_errno("Unable to create temporary file"); + if (fd < 0) { + int saved_errno = errno; + const char *nonrelative_template; + + if (!template[0]) + template = origtemplate; + + nonrelative_template = make_nonrelative_path(template); + errno = saved_errno; + die_errno("Unable to create temporary file '%s'", + nonrelative_template); + } return fd; } -- cgit v1.3-5-g9baa From 1368f65002bf39fdde7dd736a75ae35475184371 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sun, 10 Oct 2010 21:59:26 -0500 Subject: compat: helper for detecting unsigned overflow The idiom (a + b < a) works fine for detecting that an unsigned integer has overflowed, but a more explicit unsigned_add_overflows(a, b) might be easier to read. Define such a macro, expanding roughly to ((a) < UINT_MAX - (b)). Because the expansion uses each argument only once outside of sizeof() expressions, it is safe to use with arguments that have side effects. Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- git-compat-util.h | 6 ++++++ patch-delta.c | 2 +- strbuf.c | 5 +++-- wrapper.c | 2 +- 4 files changed, 11 insertions(+), 4 deletions(-) (limited to 'wrapper.c') diff --git a/git-compat-util.h b/git-compat-util.h index d6d269f138..9c23622ed5 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -31,6 +31,9 @@ #define maximum_signed_value_of_type(a) \ (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a))) +#define maximum_unsigned_value_of_type(a) \ + (UINTMAX_MAX >> (bitsizeof(uintmax_t) - bitsizeof(a))) + /* * Signed integer overflow is undefined in C, so here's a helper macro * to detect if the sum of two integers will overflow. @@ -40,6 +43,9 @@ #define signed_add_overflows(a, b) \ ((b) > maximum_signed_value_of_type(a) - (a)) +#define unsigned_add_overflows(a, b) \ + ((b) > maximum_unsigned_value_of_type(a) - (a)) + #ifdef __GNUC__ #define TYPEOF(x) (__typeof__(x)) #else diff --git a/patch-delta.c b/patch-delta.c index d218faa02b..56e0a5ede2 100644 --- a/patch-delta.c +++ b/patch-delta.c @@ -48,7 +48,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size, if (cmd & 0x20) cp_size |= (*data++ << 8); if (cmd & 0x40) cp_size |= (*data++ << 16); if (cp_size == 0) cp_size = 0x10000; - if (cp_off + cp_size < cp_size || + if (unsigned_add_overflows(cp_off, cp_size) || cp_off + cp_size > src_size || cp_size > size) break; diff --git a/strbuf.c b/strbuf.c index 9b3c4457f2..07e8883ceb 100644 --- a/strbuf.c +++ b/strbuf.c @@ -63,7 +63,8 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc) void strbuf_grow(struct strbuf *sb, size_t extra) { - if (sb->len + extra + 1 <= sb->len) + if (unsigned_add_overflows(extra, 1) || + unsigned_add_overflows(sb->len, extra + 1)) die("you want to use way too much memory"); if (!sb->alloc) sb->buf = NULL; @@ -152,7 +153,7 @@ int strbuf_cmp(const struct strbuf *a, const struct strbuf *b) void strbuf_splice(struct strbuf *sb, size_t pos, size_t len, const void *data, size_t dlen) { - if (pos + len < pos) + if (unsigned_add_overflows(pos, len)) die("you want to use way too much memory"); if (pos > sb->len) die("`pos' is too far after the end of the buffer"); diff --git a/wrapper.c b/wrapper.c index 8d7dd31c4b..79635f2e16 100644 --- a/wrapper.c +++ b/wrapper.c @@ -53,7 +53,7 @@ void *xmalloc(size_t size) void *xmallocz(size_t size) { void *ret; - if (size + 1 < size) + if (unsigned_add_overflows(size, 1)) die("Data too large to fit into virtual memory space."); ret = xmalloc(size + 1); ((char*)ret)[size] = 0; -- cgit v1.3-5-g9baa From e2a57aac8a8a2b786739a5a93ea9dcfd2f0fd0e2 Mon Sep 17 00:00:00 2001 From: Carlos Martín Nieto Date: Thu, 17 Mar 2011 12:26:46 +0100 Subject: Name make_*_path functions more accurately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename the make_*_path functions so it's clearer what they do, in particlar make clear what the differnce between make_absolute_path and make_nonrelative_path is by renaming them real_path and absolute_path respectively. make_relative_path has an understandable name and is renamed to relative_path to maintain the name convention. The function calls have been replaced 1-to-1 in their usage. Signed-off-by: Carlos Martín Nieto Signed-off-by: Junio C Hamano --- abspath.c | 18 ++++++++++++++++-- builtin/clone.c | 10 +++++----- builtin/init-db.c | 8 ++++---- builtin/receive-pack.c | 2 +- cache.h | 6 +++--- dir.c | 2 +- environment.c | 4 ++-- exec_cmd.c | 2 +- lockfile.c | 4 ++-- path.c | 2 +- setup.c | 14 +++++++------- t/t0000-basic.sh | 10 +++++----- test-path-utils.c | 4 ++-- wrapper.c | 4 ++-- 14 files changed, 52 insertions(+), 38 deletions(-) (limited to 'wrapper.c') diff --git a/abspath.c b/abspath.c index ff140689ed..3005aedde6 100644 --- a/abspath.c +++ b/abspath.c @@ -14,7 +14,14 @@ int is_directory(const char *path) /* We allow "recursive" symbolic links. Only within reason, though. */ #define MAXDEPTH 5 -const char *make_absolute_path(const char *path) +/* + * Use this to get the real path, i.e. resolve links. If you want an + * absolute path but don't mind links, use absolute_path. + * + * If path is our buffer, then return path, as it's already what the + * user wants. + */ +const char *real_path(const char *path) { static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1]; char cwd[1024] = ""; @@ -104,7 +111,14 @@ static const char *get_pwd_cwd(void) return cwd; } -const char *make_nonrelative_path(const char *path) +/* + * Use this to get an absolute path from a relative one. If you want + * to resolve links, you should use real_path. + * + * If the path is already absolute, then return path. As the user is + * never meant to free the return value, we're safe. + */ +const char *absolute_path(const char *path) { static char buf[PATH_MAX + 1]; diff --git a/builtin/clone.c b/builtin/clone.c index 2ee1fa9846..404f589680 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -100,7 +100,7 @@ static char *get_repo_path(const char *repo, int *is_bundle) path = mkpath("%s%s", repo, suffix[i]); if (is_directory(path)) { *is_bundle = 0; - return xstrdup(make_nonrelative_path(path)); + return xstrdup(absolute_path(path)); } } @@ -109,7 +109,7 @@ static char *get_repo_path(const char *repo, int *is_bundle) path = mkpath("%s%s", repo, bundle_suffix[i]); if (!stat(path, &st) && S_ISREG(st.st_mode)) { *is_bundle = 1; - return xstrdup(make_nonrelative_path(path)); + return xstrdup(absolute_path(path)); } } @@ -203,7 +203,7 @@ static void setup_reference(const char *repo) struct transport *transport; const struct ref *extra; - ref_git = make_absolute_path(option_reference); + ref_git = real_path(option_reference); if (is_directory(mkpath("%s/.git/objects", ref_git))) ref_git = mkpath("%s/.git", ref_git); @@ -411,7 +411,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) path = get_repo_path(repo_name, &is_bundle); if (path) - repo = xstrdup(make_nonrelative_path(repo_name)); + repo = xstrdup(absolute_path(repo_name)); else if (!strchr(repo_name, ':')) die("repository '%s' does not exist", repo_name); else @@ -466,7 +466,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (safe_create_leading_directories_const(git_dir) < 0) die("could not create leading directories of '%s'", git_dir); - set_git_dir(make_absolute_path(git_dir)); + set_git_dir(real_path(git_dir)); if (0 <= option_verbosity) printf("Cloning into %s%s...\n", diff --git a/builtin/init-db.c b/builtin/init-db.c index fbeb380ee2..8f5cfd7122 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -501,7 +501,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) const char *git_dir_parent = strrchr(git_dir, '/'); if (git_dir_parent) { char *rel = xstrndup(git_dir, git_dir_parent - git_dir); - git_work_tree_cfg = xstrdup(make_absolute_path(rel)); + git_work_tree_cfg = xstrdup(real_path(rel)); free(rel); } if (!git_work_tree_cfg) { @@ -510,7 +510,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) die_errno ("Cannot access current working directory"); } if (work_tree) - set_git_work_tree(make_absolute_path(work_tree)); + set_git_work_tree(real_path(work_tree)); else set_git_work_tree(git_work_tree_cfg); if (access(get_git_work_tree(), X_OK)) @@ -519,10 +519,10 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) } else { if (work_tree) - set_git_work_tree(make_absolute_path(work_tree)); + set_git_work_tree(real_path(work_tree)); } - set_git_dir(make_absolute_path(git_dir)); + set_git_dir(real_path(git_dir)); return init_db(template_dir, flags); } diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 760817dbd7..d883585804 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -740,7 +740,7 @@ static int add_refs_from_alternate(struct alternate_object_database *e, void *un const struct ref *extra; e->name[-1] = '\0'; - other = xstrdup(make_absolute_path(e->base)); + other = xstrdup(real_path(e->base)); e->name[-1] = '/'; len = strlen(other); diff --git a/cache.h b/cache.h index cbdeaa1d0d..a99fd560e9 100644 --- a/cache.h +++ b/cache.h @@ -731,9 +731,9 @@ static inline int is_absolute_path(const char *path) return path[0] == '/' || has_dos_drive_prefix(path); } int is_directory(const char *); -const char *make_absolute_path(const char *path); -const char *make_nonrelative_path(const char *path); -const char *make_relative_path(const char *abs, const char *base); +const char *real_path(const char *path); +const char *absolute_path(const char *path); +const char *relative_path(const char *abs, const char *base); int normalize_path_copy(char *dst, const char *src); int longest_ancestor_length(const char *path, const char *prefix_list); char *strip_path_suffix(const char *path, const char *suffix); diff --git a/dir.c b/dir.c index 168dad6152..325fb56ad3 100644 --- a/dir.c +++ b/dir.c @@ -1128,7 +1128,7 @@ char *get_relative_cwd(char *buffer, int size, const char *dir) die_errno("can't find the current directory"); if (!is_absolute_path(dir)) - dir = make_absolute_path(dir); + dir = real_path(dir); while (*dir && *dir == *cwd) { dir++; diff --git a/environment.c b/environment.c index c3efbb9608..cc670b1562 100644 --- a/environment.c +++ b/environment.c @@ -139,7 +139,7 @@ static int git_work_tree_initialized; void set_git_work_tree(const char *new_work_tree) { if (git_work_tree_initialized) { - new_work_tree = make_absolute_path(new_work_tree); + new_work_tree = real_path(new_work_tree); if (strcmp(new_work_tree, work_tree)) die("internal error: work tree has already been set\n" "Current worktree: %s\nNew worktree: %s", @@ -147,7 +147,7 @@ void set_git_work_tree(const char *new_work_tree) return; } git_work_tree_initialized = 1; - work_tree = xstrdup(make_absolute_path(new_work_tree)); + work_tree = xstrdup(real_path(new_work_tree)); } const char *get_git_work_tree(void) diff --git a/exec_cmd.c b/exec_cmd.c index 38545e8bfd..171e841531 100644 --- a/exec_cmd.c +++ b/exec_cmd.c @@ -89,7 +89,7 @@ static void add_path(struct strbuf *out, const char *path) if (is_absolute_path(path)) strbuf_addstr(out, path); else - strbuf_addstr(out, make_nonrelative_path(path)); + strbuf_addstr(out, absolute_path(path)); strbuf_addch(out, PATH_SEP); } diff --git a/lockfile.c b/lockfile.c index b0d74cddde..c6fb77b26f 100644 --- a/lockfile.c +++ b/lockfile.c @@ -164,10 +164,10 @@ static char *unable_to_lock_message(const char *path, int err) "If no other git process is currently running, this probably means a\n" "git process crashed in this repository earlier. Make sure no other git\n" "process is running and remove the file manually to continue.", - make_nonrelative_path(path), strerror(err)); + absolute_path(path), strerror(err)); } else strbuf_addf(&buf, "Unable to create '%s.lock': %s", - make_nonrelative_path(path), strerror(err)); + absolute_path(path), strerror(err)); return strbuf_detach(&buf, NULL); } diff --git a/path.c b/path.c index 8951333cb8..4d73cc9cd2 100644 --- a/path.c +++ b/path.c @@ -397,7 +397,7 @@ int set_shared_perm(const char *path, int mode) return 0; } -const char *make_relative_path(const char *abs, const char *base) +const char *relative_path(const char *abs, const char *base) { static char buf[PATH_MAX + 1]; int i = 0, j = 0; diff --git a/setup.c b/setup.c index 021d0133ae..03cd84f2fc 100644 --- a/setup.c +++ b/setup.c @@ -9,7 +9,7 @@ char *prefix_path(const char *prefix, int len, const char *path) const char *orig = path; char *sanitized; if (is_absolute_path(orig)) { - const char *temp = make_absolute_path(path); + const char *temp = real_path(path); sanitized = xmalloc(len + strlen(temp) + 1); strcpy(sanitized, temp); } else { @@ -221,7 +221,7 @@ void setup_work_tree(void) work_tree = get_git_work_tree(); git_dir = get_git_dir(); if (!is_absolute_path(git_dir)) - git_dir = make_absolute_path(git_dir); + git_dir = real_path(get_git_dir()); if (!work_tree || chdir(work_tree)) die("This operation must be run in a work tree"); @@ -232,7 +232,7 @@ void setup_work_tree(void) if (getenv(GIT_WORK_TREE_ENVIRONMENT)) setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1); - set_git_dir(make_relative_path(git_dir, work_tree)); + set_git_dir(relative_path(git_dir, work_tree)); initialized = 1; } @@ -312,7 +312,7 @@ const char *read_gitfile_gently(const char *path) if (!is_git_directory(dir)) die("Not a git repository: %s", dir); - path = make_absolute_path(dir); + path = real_path(dir); free(buf); return path; @@ -392,7 +392,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv, if (!prefixcmp(cwd, worktree) && cwd[strlen(worktree)] == '/') { /* cwd inside worktree */ - set_git_dir(make_absolute_path(gitdirenv)); + set_git_dir(real_path(gitdirenv)); if (chdir(worktree)) die_errno("Could not chdir to '%s'", worktree); cwd[len++] = '/'; @@ -417,7 +417,7 @@ static const char *setup_discovered_git_dir(const char *gitdir, /* --work-tree is set without --git-dir; use discovered one */ if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) { if (offset != len && !is_absolute_path(gitdir)) - gitdir = xstrdup(make_absolute_path(gitdir)); + gitdir = xstrdup(real_path(gitdir)); if (chdir(cwd)) die_errno("Could not come back to cwd"); return setup_explicit_git_dir(gitdir, cwd, len, nongit_ok); @@ -425,7 +425,7 @@ static const char *setup_discovered_git_dir(const char *gitdir, /* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */ if (is_bare_repository_cfg > 0) { - set_git_dir(offset == len ? gitdir : make_absolute_path(gitdir)); + set_git_dir(offset == len ? gitdir : real_path(gitdir)); if (chdir(cwd)) die_errno("Could not come back to cwd"); return NULL; diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 8deec75c3a..f4e8f43bae 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -435,7 +435,7 @@ test_expect_success 'update-index D/F conflict' ' test $numpath0 = 1 ' -test_expect_success SYMLINKS 'absolute path works as expected' ' +test_expect_success SYMLINKS 'real path works as expected' ' mkdir first && ln -s ../.git first/.git && mkdir second && @@ -443,14 +443,14 @@ test_expect_success SYMLINKS 'absolute path works as expected' ' mkdir third && dir="$(cd .git; pwd -P)" && dir2=third/../second/other/.git && - test "$dir" = "$(test-path-utils make_absolute_path $dir2)" && + test "$dir" = "$(test-path-utils real_path $dir2)" && file="$dir"/index && - test "$file" = "$(test-path-utils make_absolute_path $dir2/index)" && + test "$file" = "$(test-path-utils real_path $dir2/index)" && basename=blub && - test "$dir/$basename" = "$(cd .git && test-path-utils make_absolute_path "$basename")" && + test "$dir/$basename" = "$(cd .git && test-path-utils real_path "$basename")" && ln -s ../first/file .git/syml && sym="$(cd first; pwd -P)"/file && - test "$sym" = "$(test-path-utils make_absolute_path "$dir2/syml")" + test "$sym" = "$(test-path-utils real_path "$dir2/syml")" ' test_expect_success 'very long name in the index handled sanely' ' diff --git a/test-path-utils.c b/test-path-utils.c index d261398d6c..e7671593df 100644 --- a/test-path-utils.c +++ b/test-path-utils.c @@ -11,9 +11,9 @@ int main(int argc, char **argv) return 0; } - if (argc >= 2 && !strcmp(argv[1], "make_absolute_path")) { + if (argc >= 2 && !strcmp(argv[1], "real_path")) { while (argc > 2) { - puts(make_absolute_path(argv[2])); + puts(real_path(argv[2])); argc--; argv++; } diff --git a/wrapper.c b/wrapper.c index 4c147d6c48..28290002b9 100644 --- a/wrapper.c +++ b/wrapper.c @@ -209,7 +209,7 @@ int xmkstemp(char *template) if (!template[0]) template = origtemplate; - nonrelative_template = make_nonrelative_path(template); + nonrelative_template = absolute_path(template); errno = saved_errno; die_errno("Unable to create temporary file '%s'", nonrelative_template); @@ -344,7 +344,7 @@ int xmkstemp_mode(char *template, int mode) if (!template[0]) template = origtemplate; - nonrelative_template = make_nonrelative_path(template); + nonrelative_template = absolute_path(template); errno = saved_errno; die_errno("Unable to create temporary file '%s'", nonrelative_template); -- cgit v1.3-5-g9baa From 56d7c27af1f8aa7519f165f6c022732e64db3716 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 26 May 2011 12:30:27 -0400 Subject: read_in_full: always report errors The read_in_full function repeatedly calls read() to fill a buffer. If the first read() returns an error, we notify the caller by returning the error. However, if we read some data and then get an error on a subsequent read, we simply return the amount of data that we did read, and the caller is unaware of the error. This makes the tradeoff that seeing the partial data is more important than the fact that an error occurred. In practice, this is generally not the case; we care more if an error occurred, and should throw away any partial data. I audited the current callers. In most cases, this will make no difference at all, as they do: if (read_in_full(fd, buf, size) != size) error("short read"); However, it will help in a few cases: 1. In sha1_file.c:index_stream, we would fail to notice errors in the incoming stream. 2. When reading symbolic refs in resolve_ref, we would fail to notice errors and potentially use a truncated ref name. 3. In various places, we will get much better error messages. For example, callers of safe_read would erroneously print "the remote end hung up unexpectedly" instead of showing the read error. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- wrapper.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'wrapper.c') diff --git a/wrapper.c b/wrapper.c index 28290002b9..85f09df747 100644 --- a/wrapper.c +++ b/wrapper.c @@ -148,8 +148,10 @@ ssize_t read_in_full(int fd, void *buf, size_t count) while (count > 0) { ssize_t loaded = xread(fd, p, count); - if (loaded <= 0) - return total ? total : loaded; + if (loaded < 0) + return -1; + if (loaded == 0) + return total; count -= loaded; p += loaded; total += loaded; -- cgit v1.3-5-g9baa