Re: [PATCH] fix potential infinite loop given large unsigned integer

Junio C Hamano [off-list ref] writes:

quoted

Ryan Flynn [off-list ref] writes:

quoted

given n, tried to find i greater than n via i=1, iterate i *= 10.
given n sufficiently close to UINT_MAX this will overflow; which can
produce i==0, which results in an infinite loop. iteratively dividing
n /= 10 does not have this problem, and though division is slower than
multiplication this only runs once per `git format-patch
--cover-letter`

Signed-off-by: pizza <redacted>

Pizza?

This is somewhat amusing.

 - digits_in_number() is called only with opt->total that is "int";

 - opt->total is the total number of patches.

 - the return value is used like this:

     sprintf(buf, "%0*d", digits_in_number(opt->total), opt->nr);

   and opt->nr runs from 1 to opt->total; the use of "d" would be already
   wrong anyway even if you computed digits_in_number() correctly.

Perhaps we should get rid of this function altogether?

Or perhaps something stupid like this...

 builtin-log.c |    6 +++++-
 log-tree.c    |   12 +-----------
 revision.h    |    2 +-
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 3817bf1..321e8f5 100644
--- a/builtin-log.c
+++ b/builtin-log.c

@@ -1096,8 +1096,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)

       total = nr;
       if (!keep_subject && auto_number && total > 1)
               numbered = 1;
-       if (numbered)
+       if (numbered) {
+               static char num_buf[64];
               rev.total = total + start_number - 1;
+               sprintf(num_buf, "%d", rev.total);
+               rev.num_width = strlen(num_buf);
+       }
       if (in_reply_to || thread || cover_letter)
               rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
       if (in_reply_to) {

diff --git a/log-tree.c b/log-tree.c
index 6f73c17..0d32a6c 100644
--- a/log-tree.c
+++ b/log-tree.c

@@ -158,16 +158,6 @@ static void append_signoff(struct strbuf *sb, const char *signoff)

       strbuf_addch(sb, '\n');
 }

-static unsigned int digits_in_number(unsigned int number)
-{
-       unsigned int i = 10, result = 1;
-       while (i <= number) {
-               i *= 10;
-               result++;
-       }
-       return result;
-}
-
 static int has_non_ascii(const char *s)
 {
       int ch;

@@ -212,7 +202,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,

               snprintf(buffer, sizeof(buffer),
                        "Subject: [%s %0*d/%d] ",
                        opt->subject_prefix,
-                        digits_in_number(opt->total),
+                        opt->num_width,
                        opt->nr, opt->total);
               subject = buffer;
       } else if (opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {

diff --git a/revision.h b/revision.h
index fb74492..21e4d9d 100644
--- a/revision.h
+++ b/revision.h

@@ -84,7 +84,7 @@ struct rev_info {

       unsigned int    abbrev;
       enum cmit_fmt   commit_format;
       struct log_info *loginfo;
-       int             nr, total;
+       int             nr, total, num_width;
       const char      *mime_boundary;
       const char      *patch_suffix;
       int             numbered_files;