Re: [PATCH] avoid possible overflow in delta size filtering computation

From: Kjetil Barvik <barvik@broadpark.no>
Date: Mar 25, 2009, 12:15 UTC
Message-ID: <86hc1hdcj1.fsf@broadpark.no>
In-Reply-To: <alpine.LFD.2.00.0903241535010.26337@xanadu.home>

Nicolas Pitre <nico@cam.org> writes:

Show 14 quoted lines

> On a 32-bit system, the maximum possible size for an object is less than 
> 4GB, while 64-bit systems may cope with larger objects.  Due to this 
> limitation, variables holding object sizes are using an unsigned long 
> type (32 bits on 32-bit systems, or 64 bits on 64-bit systems).
>
> When large objects are encountered, and/or people play with large delta 
> depth values, it is possible for the maximum allowed delta size 
> computation to overflow, especially on a 32-bit system.  When this 
> occurs, surviving result bits may represent a value much smaller than 
> what it is supposed to be, or even zero.  This prevents some objects 
> from being deltified although they do get deltified when a smaller depth 
> limit is used.  Fix this by always performing a 64-bit multiplication.
>
> Signed-off-by: Nicolas Pitre <nico@cam.org>

  I added this patch and rerun the 2 test cases form the table where
  --depth is 20000 and 95000, and got the following result:

    --depth=20000 => file size: 19126077  delta: 73814
    --depth=95000 => file size: 19126087  delta: 73814

  So, it seems that this patch almost fixed the issue.  But notice that
  the pack file was 10 bytes larger for the --depth=95000 case.

  I made a small perl script to compare the output from 'git verify-pack
  -v' of the 2 idx/pack files, and found the following difference(1)
  (first line from --depth=20000 case, second from --depth=95000):

  fe0a6f3e971373590714dbafd087b235ea60ac00  tree   9  19  18921247  731  96a3ec5789504e6d0f90c99fb1937af1ebd58e2d
  fe0a6f3e971373590714dbafd087b235ea60ac00  tree  20  29  18921247  730  12e560f7fb28558b15e3a2008fba860f9a4b2222

  'git show fe0a6f3e971373590714dbafd087b235ea60ac00' =>

tree fe0a6f3e971373590714dbafd087b235ea60ac00

Makefile t0000-basic.sh test-lib.sh

  'git show 96a3ec5789504e6d0f90c99fb1937af1ebd58e2d' =>

tree 96a3ec5789504e6d0f90c99fb1937af1ebd58e2d

Makefile t0000-basic.sh t0100-environment-names.sh t0200-update-cache.sh t0400-ls-files.sh t0500-ls-files.sh t1000-checkout-cache.sh t1001-checkout-cache.sh test-lib.sh

  'git show 12e560f7fb28558b15e3a2008fba860f9a4b2222' =>

tree 12e560f7fb28558b15e3a2008fba860f9a4b2222

Makefile t0000-basic.sh t0100-environment-names.sh t0200-update-cache.sh t0400-ls-files.sh t0500-ls-files.sh t1000-checkout-cache.sh t1001-checkout-cache.sh test-lib.sh

  -- kjetil

  1) there was lots of lines with different offsets, all of which was 10
     larger in the --depth=95000 case.