{"thread":{"id":"10400","subject":"[PATCH 2/2] Correct some sizeof(size_t) != sizeof(unsigned long) typing errors","startedAt":"2007-10-21T05:25:37Z","lastAt":"2007-10-22T04:00:10Z","messageCount":4,"participants":["Shawn O. Pearce","René Scharfe","Pierre Habouzit"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"56747","messageId":"20071021052537.GB31927@spearce.org","threadId":"10400","inReplyTo":null,"subject":"[PATCH 2/2] Correct some sizeof(size_t) != sizeof(unsigned long) typing errors","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-21T05:25:37Z","receivedAt":"2007-10-21T05:25:37Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On at least one system I've used recently sizeof(size_t) == 4\nand sizeof(unsigned long) == 8.  Trying to pass a pointer to an 8\nbyte value into strbuf_detach() causes problems as the function is\nexpecting an address for a 4 byte result location.  Writing only 4\nbytes here will leave the other 4 bytes unitialized and may cause\nproblems when the caller evalutes the result.\n\nI am introducing strbuf_detach_ul() as a variant that takes its\nsize as an unsigned long rather than as a size_t.  This approach is\nshorter than fixing all of the callers to use their own temporary\nsize_t value for the call.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n builtin-apply.c   |    2 +-\n builtin-archive.c |    2 +-\n diff.c            |    4 ++--\n entry.c           |    2 +-\n strbuf.h          |    8 +++++++-\n test-delta.c      |    3 ++-\n 6 files changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 05c6bc3..022f916 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1955,7 +1955,7 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *\n \n \tif (apply_fragments(&buf, patch) < 0)\n \t\treturn -1; /* note with --reject this succeeds. */\n-\tpatch->result = strbuf_detach(&buf, &patch->resultsize);\n+\tpatch->result = strbuf_detach_ul(&buf, &patch->resultsize);\n \n \tif (0 < patch->is_delete && patch->resultsize)\n \t\treturn error(\"removal patch leaves file contents\");\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 04385de..46d5de0 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -153,7 +153,7 @@ void *sha1_file_to_archive(const char *path, const unsigned char *sha1,\n \t\tstrbuf_attach(&buf, buffer, *sizep, *sizep + 1);\n \t\tconvert_to_working_tree(path, buf.buf, buf.len, &buf);\n \t\tconvert_to_archive(path, buf.buf, buf.len, &buf, commit);\n-\t\tbuffer = strbuf_detach(&buf, sizep);\n+\t\tbuffer = strbuf_detach_ul(&buf, sizep);\n \t}\n \n \treturn buffer;\ndiff --git a/diff.c b/diff.c\nindex 6648e01..6fd0c0a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1519,7 +1519,7 @@ static int populate_from_stdin(struct diff_filespec *s)\n \t\t\t\t     strerror(errno));\n \n \ts->should_munmap = 0;\n-\ts->data = strbuf_detach(&buf, &s->size);\n+\ts->data = strbuf_detach_ul(&buf, &s->size);\n \ts->should_free = 1;\n \treturn 0;\n }\n@@ -1611,7 +1611,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\tif (convert_to_git(s->path, s->data, s->size, &buf)) {\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\n-\t\t\ts->data = strbuf_detach(&buf, &s->size);\n+\t\t\ts->data = strbuf_detach_ul(&buf, &s->size);\n \t\t\ts->should_free = 1;\n \t\t}\n \t}\ndiff --git a/entry.c b/entry.c\nindex 98f5f6d..d36a0bb 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -120,7 +120,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\tstrbuf_init(&buf, 0);\n \t\tif (convert_to_working_tree(ce->name, new, size, &buf)) {\n \t\t\tfree(new);\n-\t\t\tnew = strbuf_detach(&buf, &size);\n+\t\t\tnew = strbuf_detach_ul(&buf, &size);\n \t\t}\n \n \t\tif (to_tempfile) {\ndiff --git a/strbuf.h b/strbuf.h\nindex 9b9e861..d6d6bd0 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -52,7 +52,13 @@ struct strbuf {\n /*----- strbuf life cycle -----*/\n extern void strbuf_init(struct strbuf *, size_t);\n extern void strbuf_release(struct strbuf *);\n-extern char *strbuf_detach(struct strbuf *, size_t *);\n+extern char *strbuf_detach(struct strbuf *, unsigned long *);\n+static inline char *strbuf_detach_ul(struct strbuf *a, unsigned long *n) {\n+\tsize_t len;\n+\tchar *res = strbuf_detach(a, &len);\n+\t*n = len;\n+\treturn res;\n+}\n extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);\n static inline void strbuf_swap(struct strbuf *a, struct strbuf *b) {\n \tstruct strbuf tmp = *a;\ndiff --git a/test-delta.c b/test-delta.c\nindex 3d885ff..018e7dc 100644\n--- a/test-delta.c\n+++ b/test-delta.c\n@@ -20,7 +20,8 @@ int main(int argc, char *argv[])\n \tint fd;\n \tstruct stat st;\n \tvoid *from_buf, *data_buf, *out_buf;\n-\tunsigned long from_size, data_size, out_size;\n+\tunsigned long from_size, data_size;\n+\tsize_t out_size;\n \n \tif (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\"))) {\n \t\tfprintf(stderr, \"Usage: %s\\n\", usage_str);\n-- \n1.5.3.4.1270.g2fe543\n"},{"id":"56761","messageId":"471B1AA5.8070009@lsrfire.ath.cx","threadId":"10400","inReplyTo":"20071021052537.GB31927@spearce.org","subject":"Re: [PATCH 2/2] Correct some sizeof(size_t) != sizeof(unsigned long) typing errors","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-10-21T09:23:49Z","receivedAt":"2007-10-21T09:23:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Fix size_t vs. unsigned long pointer mismatch warnings introduced\nwith the addition of strbuf_detach().\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\nShawn O. Pearce schrieb:\n> On at least one system I've used recently sizeof(size_t) == 4\n> and sizeof(unsigned long) == 8.  Trying to pass a pointer to an 8\n> byte value into strbuf_detach() causes problems as the function is\n> expecting an address for a 4 byte result location.  Writing only 4\n> bytes here will leave the other 4 bytes unitialized and may cause\n> problems when the caller evalutes the result.\n> \n> I am introducing strbuf_detach_ul() as a variant that takes its\n> size as an unsigned long rather than as a size_t.  This approach is\n> shorter than fixing all of the callers to use their own temporary\n> size_t value for the call.\n> \n> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>\n> ---\n>  builtin-apply.c   |    2 +-\n>  builtin-archive.c |    2 +-\n>  diff.c            |    4 ++--\n>  entry.c           |    2 +-\n>  strbuf.h          |    8 +++++++-\n>  test-delta.c      |    3 ++-\n>  6 files changed, 14 insertions(+), 7 deletions(-)\n\nI have a feeling this is going in then wrong direction.  Shouldn't\nwe rather use size_t everywhere?  malloc() takes a size_t, and it's\nthe basis of strbuf and also of the file content functions.\n\nThe following patch is shorter, though it adds one more line than\nyour's.  The result is slightly uglier, but it's a good reminder to\nuse size_t in more places.\n\nThat said, I realize that converting read_sha1_file() et al. would\nbe quite painful.  Maybe too painful?\n\n builtin-apply.c   |    2 +-\n builtin-archive.c |    4 +++-\n diff.c            |    8 ++++++--\n entry.c           |    4 +++-\n 4 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 047a60d..541a6f4 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -152,7 +152,7 @@ struct patch {\n \tunsigned int is_rename:1;\n \tstruct fragment *fragments;\n \tchar *result;\n-\tunsigned long resultsize;\n+\tsize_t resultsize;\n \tchar old_sha1_prefix[41];\n \tchar new_sha1_prefix[41];\n \tstruct patch *next;\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 04385de..6f29c2f 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -148,12 +148,14 @@ void *sha1_file_to_archive(const char *path, const unsigned char *sha1,\n \tbuffer = read_sha1_file(sha1, type, sizep);\n \tif (buffer && S_ISREG(mode)) {\n \t\tstruct strbuf buf;\n+\t\tsize_t size = 0;\n \n \t\tstrbuf_init(&buf, 0);\n \t\tstrbuf_attach(&buf, buffer, *sizep, *sizep + 1);\n \t\tconvert_to_working_tree(path, buf.buf, buf.len, &buf);\n \t\tconvert_to_archive(path, buf.buf, buf.len, &buf, commit);\n-\t\tbuffer = strbuf_detach(&buf, sizep);\n+\t\tbuffer = strbuf_detach(&buf, &size);\n+\t\t*sizep = size;\n \t}\n \n \treturn buffer;\ndiff --git a/diff.c b/diff.c\nindex 0bd7e24..df82ed6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1512,6 +1512,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n static int populate_from_stdin(struct diff_filespec *s)\n {\n \tstruct strbuf buf;\n+\tsize_t size = 0;\n \n \tstrbuf_init(&buf, 0);\n \tif (strbuf_read(&buf, 0, 0) < 0)\n@@ -1519,7 +1520,8 @@ static int populate_from_stdin(struct diff_filespec *s)\n \t\t\t\t     strerror(errno));\n \n \ts->should_munmap = 0;\n-\ts->data = strbuf_detach(&buf, &s->size);\n+\ts->data = strbuf_detach(&buf, &size);\n+\ts->size = size;\n \ts->should_free = 1;\n \treturn 0;\n }\n@@ -1609,9 +1611,11 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\t */\n \t\tstrbuf_init(&buf, 0);\n \t\tif (convert_to_git(s->path, s->data, s->size, &buf)) {\n+\t\t\tsize_t size = 0;\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\n-\t\t\ts->data = strbuf_detach(&buf, &s->size);\n+\t\t\ts->data = strbuf_detach(&buf, &size);\n+\t\t\ts->size = size;\n \t\t\ts->should_free = 1;\n \t\t}\n \t}\ndiff --git a/entry.c b/entry.c\nindex 98f5f6d..cfadc6a 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -119,8 +119,10 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\t */\n \t\tstrbuf_init(&buf, 0);\n \t\tif (convert_to_working_tree(ce->name, new, size, &buf)) {\n+\t\t\tsize_t newsize = 0;\n \t\t\tfree(new);\n-\t\t\tnew = strbuf_detach(&buf, &size);\n+\t\t\tnew = strbuf_detach(&buf, &newsize);\n+\t\t\tsize = newsize;\n \t\t}\n \n \t\tif (to_tempfile) {\n"},{"id":"56767","messageId":"20071021103132.GA25741@artemis.corp","threadId":"10400","inReplyTo":"471B1AA5.8070009@lsrfire.ath.cx","subject":"Re: [PATCH 2/2] Correct some sizeof(size_t) != sizeof(unsigned long) typing errors","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-21T10:31:32Z","receivedAt":"2007-10-21T10:31:32Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sun, Oct 21, 2007 at 09:23:49AM +0000, René Scharfe wrote:\n> > Signed-off-by: Shawn O. Pearce <spearce@spearce.org>\n> > ---\n> >  builtin-apply.c   |    2 +-\n> >  builtin-archive.c |    2 +-\n> >  diff.c            |    4 ++--\n> >  entry.c           |    2 +-\n> >  strbuf.h          |    8 +++++++-\n> >  test-delta.c      |    3 ++-\n> >  6 files changed, 14 insertions(+), 7 deletions(-)\n> \n> I have a feeling this is going in then wrong direction.  Shouldn't\n> we rather use size_t everywhere?  malloc() takes a size_t, and it's\n> the basis of strbuf and also of the file content functions.\n\n  I agree, Junio was working on a patch that generalized use of size_t's\nwhen unsigned long where used and size_t meant, I suppose he didn't had\nthe time to push it.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"56797","messageId":"20071022040010.GH14735@spearce.org","threadId":"10400","inReplyTo":"20071021103132.GA25741@artemis.corp","subject":"Re: [PATCH 2/2] Correct some sizeof(size_t) != sizeof(unsigned long) typing errors","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-22T04:00:10Z","receivedAt":"2007-10-22T04:00:10Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Pierre Habouzit <madcoder@debian.org> wrote:\n> On Sun, Oct 21, 2007 at 09:23:49AM +0000, René Scharfe wrote:\n> > > Signed-off-by: Shawn O. Pearce <spearce@spearce.org>\n> > > ---\n> > >  builtin-apply.c   |    2 +-\n> > >  builtin-archive.c |    2 +-\n> > >  diff.c            |    4 ++--\n> > >  entry.c           |    2 +-\n> > >  strbuf.h          |    8 +++++++-\n> > >  test-delta.c      |    3 ++-\n> > >  6 files changed, 14 insertions(+), 7 deletions(-)\n> > \n> > I have a feeling this is going in then wrong direction.  Shouldn't\n> > we rather use size_t everywhere?  malloc() takes a size_t, and it's\n> > the basis of strbuf and also of the file content functions.\n> \n>   I agree, Junio was working on a patch that generalized use of size_t's\n> when unsigned long where used and size_t meant, I suppose he didn't had\n> the time to push it.\n\nYea, you guys convinced me to go with René's patch.  I'm\nreplacing mine and will put it into next tonight.\n\nI actually had started with what René wrote but changed it to\nwhat you saw before posting it to the list.  :-)\n\n-- \nShawn.\n"}]}