[PATCH 2/2] Correct some sizeof(size_t) != sizeof(unsigned long) typing errors

[PATCH 2/2] Correct some sizeof(size_t) != sizeof(unsigned long) typing errors

[Shawn O. Pearce]

On at least one system I've used recently sizeof(size_t) == 4
and sizeof(unsigned long) == 8.  Trying to pass a pointer to an 8
byte value into strbuf_detach() causes problems as the function is
expecting an address for a 4 byte result location.  Writing only 4
bytes here will leave the other 4 bytes unitialized and may cause
problems when the caller evalutes the result.

I am introducing strbuf_detach_ul() as a variant that takes its
size as an unsigned long rather than as a size_t.  This approach is
shorter than fixing all of the callers to use their own temporary
size_t value for the call.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-apply.c   |    2 +-
 builtin-archive.c |    2 +-
 diff.c            |    4 ++--
 entry.c           |    2 +-
 strbuf.h          |    8 +++++++-
 test-delta.c      |    3 ++-
 6 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 05c6bc3..022f916 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1955,7 +1955,7 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
 
 	if (apply_fragments(&buf, patch) < 0)
 		return -1; /* note with --reject this succeeds. */
-	patch->result = strbuf_detach(&buf, &patch->resultsize);
+	patch->result = strbuf_detach_ul(&buf, &patch->resultsize);
 
 	if (0 < patch->is_delete && patch->resultsize)
 		return error("removal patch leaves file contents");
diff --git a/builtin-archive.c b/builtin-archive.c
index 04385de..46d5de0 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -153,7 +153,7 @@ void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
 		strbuf_attach(&buf, buffer, *sizep, *sizep + 1);
 		convert_to_working_tree(path, buf.buf, buf.len, &buf);
 		convert_to_archive(path, buf.buf, buf.len, &buf, commit);
-		buffer = strbuf_detach(&buf, sizep);
+		buffer = strbuf_detach_ul(&buf, sizep);
 	}
 
 	return buffer;
diff --git a/diff.c b/diff.c
index 6648e01..6fd0c0a 100644
--- a/diff.c
+++ b/diff.c
@@ -1519,7 +1519,7 @@ static int populate_from_stdin(struct diff_filespec *s)
 				     strerror(errno));
 
 	s->should_munmap = 0;
-	s->data = strbuf_detach(&buf, &s->size);
+	s->data = strbuf_detach_ul(&buf, &s->size);
 	s->should_free = 1;
 	return 0;
 }
@@ -1611,7 +1611,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		if (convert_to_git(s->path, s->data, s->size, &buf)) {
 			munmap(s->data, s->size);
 			s->should_munmap = 0;
-			s->data = strbuf_detach(&buf, &s->size);
+			s->data = strbuf_detach_ul(&buf, &s->size);
 			s->should_free = 1;
 		}
 	}
diff --git a/entry.c b/entry.c
index 98f5f6d..d36a0bb 100644
--- a/entry.c
+++ b/entry.c
@@ -120,7 +120,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 		strbuf_init(&buf, 0);
 		if (convert_to_working_tree(ce->name, new, size, &buf)) {
 			free(new);
-			new = strbuf_detach(&buf, &size);
+			new = strbuf_detach_ul(&buf, &size);
 		}
 
 		if (to_tempfile) {
diff --git a/strbuf.h b/strbuf.h
index 9b9e861..d6d6bd0 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -52,7 +52,13 @@ struct strbuf {
 /*----- strbuf life cycle -----*/
 extern void strbuf_init(struct strbuf *, size_t);
 extern void strbuf_release(struct strbuf *);
-extern char *strbuf_detach(struct strbuf *, size_t *);
+extern char *strbuf_detach(struct strbuf *, unsigned long *);
+static inline char *strbuf_detach_ul(struct strbuf *a, unsigned long *n) {
+	size_t len;
+	char *res = strbuf_detach(a, &len);
+	*n = len;
+	return res;
+}
 extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);
 static inline void strbuf_swap(struct strbuf *a, struct strbuf *b) {
 	struct strbuf tmp = *a;
diff --git a/test-delta.c b/test-delta.c
index 3d885ff..018e7dc 100644
--- a/test-delta.c
+++ b/test-delta.c
@@ -20,7 +20,8 @@ int main(int argc, char *argv[])
 	int fd;
 	struct stat st;
 	void *from_buf, *data_buf, *out_buf;
-	unsigned long from_size, data_size, out_size;
+	unsigned long from_size, data_size;
+	size_t out_size;
 
 	if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p"))) {
 		fprintf(stderr, "Usage: %s\n", usage_str);
-- 
1.5.3.4.1270.g2fe543

Re: [PATCH 2/2] Correct some sizeof(size_t) != sizeof(unsigned long) typing errors

[René Scharfe]

Fix size_t vs. unsigned long pointer mismatch warnings introduced
with the addition of strbuf_detach().

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
Shawn O. Pearce schrieb:
> On at least one system I've used recently sizeof(size_t) == 4
> and sizeof(unsigned long) == 8.  Trying to pass a pointer to an 8
> byte value into strbuf_detach() causes problems as the function is
> expecting an address for a 4 byte result location.  Writing only 4
> bytes here will leave the other 4 bytes unitialized and may cause
> problems when the caller evalutes the result.
> 
> I am introducing strbuf_detach_ul() as a variant that takes its
> size as an unsigned long rather than as a size_t.  This approach is
> shorter than fixing all of the callers to use their own temporary
> size_t value for the call.
> 
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>  builtin-apply.c   |    2 +-
>  builtin-archive.c |    2 +-
>  diff.c            |    4 ++--
>  entry.c           |    2 +-
>  strbuf.h          |    8 +++++++-
>  test-delta.c      |    3 ++-
>  6 files changed, 14 insertions(+), 7 deletions(-)

I have a feeling this is going in then wrong direction.  Shouldn't
we rather use size_t everywhere?  malloc() takes a size_t, and it's
the basis of strbuf and also of the file content functions.

The following patch is shorter, though it adds one more line than
your's.  The result is slightly uglier, but it's a good reminder to
use size_t in more places.

That said, I realize that converting read_sha1_file() et al. would
be quite painful.  Maybe too painful?

 builtin-apply.c   |    2 +-
 builtin-archive.c |    4 +++-
 diff.c            |    8 ++++++--
 entry.c           |    4 +++-
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 047a60d..541a6f4 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -152,7 +152,7 @@ struct patch {
 	unsigned int is_rename:1;
 	struct fragment *fragments;
 	char *result;
-	unsigned long resultsize;
+	size_t resultsize;
 	char old_sha1_prefix[41];
 	char new_sha1_prefix[41];
 	struct patch *next;
diff --git a/builtin-archive.c b/builtin-archive.c
index 04385de..6f29c2f 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -148,12 +148,14 @@ void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
 	buffer = read_sha1_file(sha1, type, sizep);
 	if (buffer && S_ISREG(mode)) {
 		struct strbuf buf;
+		size_t size = 0;
 
 		strbuf_init(&buf, 0);
 		strbuf_attach(&buf, buffer, *sizep, *sizep + 1);
 		convert_to_working_tree(path, buf.buf, buf.len, &buf);
 		convert_to_archive(path, buf.buf, buf.len, &buf, commit);
-		buffer = strbuf_detach(&buf, sizep);
+		buffer = strbuf_detach(&buf, &size);
+		*sizep = size;
 	}
 
 	return buffer;
diff --git a/diff.c b/diff.c
index 0bd7e24..df82ed6 100644
--- a/diff.c
+++ b/diff.c
@@ -1512,6 +1512,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
 static int populate_from_stdin(struct diff_filespec *s)
 {
 	struct strbuf buf;
+	size_t size = 0;
 
 	strbuf_init(&buf, 0);
 	if (strbuf_read(&buf, 0, 0) < 0)
@@ -1519,7 +1520,8 @@ static int populate_from_stdin(struct diff_filespec *s)
 				     strerror(errno));
 
 	s->should_munmap = 0;
-	s->data = strbuf_detach(&buf, &s->size);
+	s->data = strbuf_detach(&buf, &size);
+	s->size = size;
 	s->should_free = 1;
 	return 0;
 }
@@ -1609,9 +1611,11 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		 */
 		strbuf_init(&buf, 0);
 		if (convert_to_git(s->path, s->data, s->size, &buf)) {
+			size_t size = 0;
 			munmap(s->data, s->size);
 			s->should_munmap = 0;
-			s->data = strbuf_detach(&buf, &s->size);
+			s->data = strbuf_detach(&buf, &size);
+			s->size = size;
 			s->should_free = 1;
 		}
 	}
diff --git a/entry.c b/entry.c
index 98f5f6d..cfadc6a 100644
--- a/entry.c
+++ b/entry.c
@@ -119,8 +119,10 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 		 */
 		strbuf_init(&buf, 0);
 		if (convert_to_working_tree(ce->name, new, size, &buf)) {
+			size_t newsize = 0;
 			free(new);
-			new = strbuf_detach(&buf, &size);
+			new = strbuf_detach(&buf, &newsize);
+			size = newsize;
 		}
 
 		if (to_tempfile) {

Re: [PATCH 2/2] Correct some sizeof(size_t) != sizeof(unsigned long) typing errors

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

On Sun, Oct 21, 2007 at 09:23:49AM +0000, René Scharfe wrote:
> > Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> > ---
> >  builtin-apply.c   |    2 +-
> >  builtin-archive.c |    2 +-
> >  diff.c            |    4 ++--
> >  entry.c           |    2 +-
> >  strbuf.h          |    8 +++++++-
> >  test-delta.c      |    3 ++-
> >  6 files changed, 14 insertions(+), 7 deletions(-)
> 
> I have a feeling this is going in then wrong direction.  Shouldn't
> we rather use size_t everywhere?  malloc() takes a size_t, and it's
> the basis of strbuf and also of the file content functions.

  I agree, Junio was working on a patch that generalized use of size_t's
when unsigned long where used and size_t meant, I suppose he didn't had
the time to push it.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/2] Correct some sizeof(size_t) != sizeof(unsigned long) typing errors

[Shawn O. Pearce]

Pierre Habouzit <madcoder@debian.org> wrote:
> On Sun, Oct 21, 2007 at 09:23:49AM +0000, René Scharfe wrote:
> > > Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> > > ---
> > >  builtin-apply.c   |    2 +-
> > >  builtin-archive.c |    2 +-
> > >  diff.c            |    4 ++--
> > >  entry.c           |    2 +-
> > >  strbuf.h          |    8 +++++++-
> > >  test-delta.c      |    3 ++-
> > >  6 files changed, 14 insertions(+), 7 deletions(-)
> > 
> > I have a feeling this is going in then wrong direction.  Shouldn't
> > we rather use size_t everywhere?  malloc() takes a size_t, and it's
> > the basis of strbuf and also of the file content functions.
> 
>   I agree, Junio was working on a patch that generalized use of size_t's
> when unsigned long where used and size_t meant, I suppose he didn't had
> the time to push it.

Yea, you guys convinced me to go with René's patch.  I'm
replacing mine and will put it into next tonight.

I actually had started with what René wrote but changed it to
what you saw before posting it to the list.  :-)

-- 
Shawn.