Re: [PATCH 23/67] add_packed_git: convert strcpy into xsnprintf

[Junio C Hamano]

Jeff King  writes:

> + alloc = path_len + strlen(".pack") + 1;
> + p = alloc_packed_git(alloc);
> + memcpy(p->pack_name, path, path_len); /* NUL from zero-ed struct */

This comment is confusing, isn't it?  Yes, there is a NUL, but you
will going to overwrite it with "." in ".keep" immediately and more
importantly, that overwriting does not depend on NUL being there.

What's more important to comment on would probably be the line that
computes the "alloc".  It uses ".pack" but that is because it knows
that is the longest suffix we care about, and that deserves mention
more than the NUL termination of intermediate result that does not
matter, no?

If ".bitmap" were in the set of suffixes we care about (it isn't),
we would be using that instead when measuring "alloc".

Thanks.

> +
> + xsnprintf(p->pack_name + path_len, alloc - path_len, ".keep");
>   if (!access(p->pack_name, F_OK))
>   p->pack_keep = 1;
>  
> - strcpy(p->pack_name + path_len, ".pack");
> + xsnprintf(p->pack_name + path_len, alloc - path_len, ".pack");
>   if (stat(p->pack_name, ) || !S_ISREG(st.st_mode)) {
>   free(p);
>   return NULL;
--
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 23/67] add_packed_git: convert strcpy into xsnprintf

[Jeff King]

On Wed, Sep 16, 2015 at 11:43:49AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > +   alloc = path_len + strlen(".pack") + 1;
> > +   p = alloc_packed_git(alloc);
> > +   memcpy(p->pack_name, path, path_len); /* NUL from zero-ed struct */
> 
> This comment is confusing, isn't it?  Yes, there is a NUL, but you
> will going to overwrite it with "." in ".keep" immediately and more
> importantly, that overwriting does not depend on NUL being there.

Yeah, you're right. I was blindly making sure that the behavior did not
change from the original, without noticing that the original did not
care about the NUL either way.

> What's more important to comment on would probably be the line that
> computes the "alloc".  It uses ".pack" but that is because it knows
> that is the longest suffix we care about, and that deserves mention
> more than the NUL termination of intermediate result that does not
> matter, no?

Agreed. I'll add a comment to that effect.

-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 23/67] add_packed_git: convert strcpy into xsnprintf

[Jeff King]

We have the path "foo.idx", and we create a buffer big
enough to hold "foo.pack" and "foo.keep", and then strcpy
straight into it. This isn't a bug (we have enough space),
but it's very hard to tell from the strcpy that this is so.

Let's instead use strip_suffix to take off the ".idx",
record the size of our allocation, and use xsnprintf to make
sure we don't violate our assumptions.

Signed-off-by: Jeff King 
---
 cache.h |  2 +-
 sha1_file.c | 19 ++-
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index cc59aba..11372ef 100644
--- a/cache.h
+++ b/cache.h
@@ -1305,7 +1305,7 @@ extern void close_pack_windows(struct packed_git *);
 extern void unuse_pack(struct pack_window **);
 extern void free_pack_by_name(const char *);
 extern void clear_delta_base_cache(void);
-extern struct packed_git *add_packed_git(const char *, int, int);
+extern struct packed_git *add_packed_git(const char *path, size_t path_len, 
int local);
 
 /*
  * Return the SHA-1 of the nth object within the specified packfile.
diff --git a/sha1_file.c b/sha1_file.c
index f106091..28352a5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1146,11 +1146,12 @@ static void try_to_free_pack_memory(size_t size)
release_pack_memory(size);
 }
 
-struct packed_git *add_packed_git(const char *path, int path_len, int local)
+struct packed_git *add_packed_git(const char *path, size_t 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);
+   size_t alloc;
+   struct packed_git *p;
 
if (!have_set_try_to_free_routine) {
have_set_try_to_free_routine = 1;
@@ -1161,18 +1162,18 @@ struct packed_git *add_packed_git(const char *path, int 
path_len, int local)
 * Make sure a corresponding .pack file exists and that
 * the index looks sane.
 */
-   path_len -= strlen(".idx");
-   if (path_len < 1) {
-   free(p);
+   if (!strip_suffix_mem(path, _len, ".idx"))
return NULL;
-   }
-   memcpy(p->pack_name, path, path_len);
 
-   strcpy(p->pack_name + path_len, ".keep");
+   alloc = path_len + strlen(".pack") + 1;
+   p = alloc_packed_git(alloc);
+   memcpy(p->pack_name, path, path_len); /* NUL from zero-ed struct */
+
+   xsnprintf(p->pack_name + path_len, alloc - path_len, ".keep");
if (!access(p->pack_name, F_OK))
p->pack_keep = 1;
 
-   strcpy(p->pack_name + path_len, ".pack");
+   xsnprintf(p->pack_name + path_len, alloc - path_len, ".pack");
if (stat(p->pack_name, ) || !S_ISREG(st.st_mode)) {
free(p);
return NULL;
-- 
2.6.0.rc2.408.ga2926b9

--
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