Re: [PATCH] fix potential infinite loop given large unsigned integer
On Sun, Aug 9, 2009 at 3:38 AM, Junio C Hamano<gitster@pobox.com> wrote:
Show 99 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ryan Flynn <parseerror@gmail.com> writes:
>>
>>> 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 <parseerror@gmail.com>
>>
>> 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;
>why carry around a piece of information that is only used in one place and is not expensive to calculate? how about a middle-ground such as:
Show changes to log-tree.c +4 −7
diff --git a/log-tree.c b/log-tree.c
index 6f73c17..4888518 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -158,14 +158,11 @@ static void append_signoff(struct strbuf *sb,
const char *signoff)
strbuf_addch(sb, '\n');
}
-static unsigned int digits_in_number(unsigned int number)
+static int digits_in_number(int number)
{
- unsigned int i = 10, result = 1;
- while (i <= number) {
- i *= 10;
- result++;
- }
- return result;
+ static char num_buf[64];
+ sprintf(num_buf, "%u", number);
+ return (int)strlen(num_buf);
}
static int has_non_ascii(const char *s)
Ryan Flynn
