Re: [PATCH v7 2/5] setup: sanity check file size in read_gitfile_gently
On Sun, Jun 14, 2015 at 1:21 PM, erik elfström wrote: > On Sun, Jun 14, 2015 at 5:42 AM, Eric Sunshine > wrote: >> >> This variable name doesn't convey much about its purpose, and >> introduces a bit of maintenance burden if the limit is some day >> changed. Perhaps "sane_size_limit" or something even more descriptive >> (and/or terse) would be better. > > Would you be happy with this change? > > - static const int one_MB = 1 << 20; > + static const int max_file_size = 1 << 20; /* 1MB */ Yep, that's a much nicer variable name. Thanks. I also note that 'const int' shows up pretty frequently in the git source code, but 'static const int' is used only very rarely. -- 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 v7 2/5] setup: sanity check file size in read_gitfile_gently
On Sun, Jun 14, 2015 at 5:42 AM, Eric Sunshine wrote: > > This variable name doesn't convey much about its purpose, and > introduces a bit of maintenance burden if the limit is some day > changed. Perhaps "sane_size_limit" or something even more descriptive > (and/or terse) would be better. > Would you be happy with this change? - static const int one_MB = 1 << 20; + static const int max_file_size = 1 << 20; /* 1MB */ -- 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 v7 2/5] setup: sanity check file size in read_gitfile_gently
On Tue, Jun 9, 2015 at 2:24 PM, Erik Elfström wrote:
> read_gitfile_gently will allocate a buffer to fit the entire file that
> should be read. Add a sanity check of the file size before opening to
> avoid allocating a potentially huge amount of memory if we come across
> a large file that someone happened to name ".git". The limit is set to
> a sufficiently unreasonable size that should never be exceeded by a
> genuine .git file.
>
> Signed-off-by: Erik Elfström
> ---
> diff --git a/setup.c b/setup.c
> index 4748b63..e76955e 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -414,6 +414,7 @@ static void update_linked_gitdir(const char *gitfile,
> const char *gitdir)
> */
> const char *read_gitfile_gently(const char *path, int *return_error_code)
> {
> + static const int one_MB = 1 << 20;
This variable name doesn't convey much about its purpose, and
introduces a bit of maintenance burden if the limit is some day
changed. Perhaps "sane_size_limit" or something even more descriptive
(and/or terse) would be better.
> int error_code = 0;
> char *buf = NULL;
> char *dir = NULL;
> @@ -430,6 +431,10 @@ const char *read_gitfile_gently(const char *path, int
> *return_error_code)
> error_code = READ_GITFILE_ERR_NOT_A_FILE;
> goto cleanup_return;
> }
> + if (st.st_size > one_MB) {
> + error_code = READ_GITFILE_ERR_TOO_LARGE;
> + goto cleanup_return;
> + }
> fd = open(path, O_RDONLY);
> if (fd < 0) {
> error_code = READ_GITFILE_ERR_OPEN_FAILED;
> @@ -489,6 +494,8 @@ cleanup_return:
> return NULL;
> case READ_GITFILE_ERR_OPEN_FAILED:
> die_errno("Error opening '%s'", path);
> + case READ_GITFILE_ERR_TOO_LARGE:
> + die("Too large to be a .git file: '%s'", path);
> case READ_GITFILE_ERR_READ_FAILED:
> die("Error reading %s", path);
> case READ_GITFILE_ERR_INVALID_FORMAT:
> --
> 2.4.3.373.gc496bfb
--
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 v7 2/5] setup: sanity check file size in read_gitfile_gently
read_gitfile_gently will allocate a buffer to fit the entire file that
should be read. Add a sanity check of the file size before opening to
avoid allocating a potentially huge amount of memory if we come across
a large file that someone happened to name ".git". The limit is set to
a sufficiently unreasonable size that should never be exceeded by a
genuine .git file.
Signed-off-by: Erik Elfström
---
cache.h | 1 +
setup.c | 7 +++
2 files changed, 8 insertions(+)
diff --git a/cache.h b/cache.h
index 25578cb..858d9b3 100644
--- a/cache.h
+++ b/cache.h
@@ -454,6 +454,7 @@ extern const char *get_git_work_tree(void);
#define READ_GITFILE_ERR_INVALID_FORMAT 5
#define READ_GITFILE_ERR_NO_PATH 6
#define READ_GITFILE_ERR_NOT_A_REPO 7
+#define READ_GITFILE_ERR_TOO_LARGE 8
extern const char *read_gitfile_gently(const char *path, int
*return_error_code);
#define read_gitfile(path) read_gitfile_gently((path), NULL)
extern const char *resolve_gitdir(const char *suspect);
diff --git a/setup.c b/setup.c
index 4748b63..e76955e 100644
--- a/setup.c
+++ b/setup.c
@@ -414,6 +414,7 @@ static void update_linked_gitdir(const char *gitfile, const
char *gitdir)
*/
const char *read_gitfile_gently(const char *path, int *return_error_code)
{
+ static const int one_MB = 1 << 20;
int error_code = 0;
char *buf = NULL;
char *dir = NULL;
@@ -430,6 +431,10 @@ const char *read_gitfile_gently(const char *path, int
*return_error_code)
error_code = READ_GITFILE_ERR_NOT_A_FILE;
goto cleanup_return;
}
+ if (st.st_size > one_MB) {
+ error_code = READ_GITFILE_ERR_TOO_LARGE;
+ goto cleanup_return;
+ }
fd = open(path, O_RDONLY);
if (fd < 0) {
error_code = READ_GITFILE_ERR_OPEN_FAILED;
@@ -489,6 +494,8 @@ cleanup_return:
return NULL;
case READ_GITFILE_ERR_OPEN_FAILED:
die_errno("Error opening '%s'", path);
+ case READ_GITFILE_ERR_TOO_LARGE:
+ die("Too large to be a .git file: '%s'", path);
case READ_GITFILE_ERR_READ_FAILED:
die("Error reading %s", path);
case READ_GITFILE_ERR_INVALID_FORMAT:
--
2.4.3.373.gc496bfb
--
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
