Re: [PATCH] Fix delta integer overflows
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 7, 2017, 19:39 UTC
- Message-ID
- <alpine.DEB.2.21.1.1708072136290.4271@virtualbox>
- In-Reply-To
- <xmqq1sonql76.fsf@gitster.mtv.corp.google.com>
Hi,
On Mon, 7 Aug 2017, Junio C Hamano wrote:
Show 26 quoted lines
> Martin Koegler <martin.koegler@chello.at> writes: > > > From: Martin Koegler <martin.koegler@chello.at> > > > > The current delta code produces incorrect pack objects for files > 4GB. > > > > Signed-off-by: Martin Koegler <martin.koegler@chello.at> > > --- > > diff-delta.c | 23 ++++++++++++----------- > > 1 file changed, 12 insertions(+), 11 deletions(-) > > > > Just pass any file > 4 GB to the delta-compression [by increasing the delta limits]. > > As file size, a truncated 32bit value will be encoded, leading to broken pack files. > > The patch obviously makes the code better and self consistent in > that "struct delta_index" has src_size as ulong, and this function > takes trg_size as ulong, and it was plain wrong for the code to > assume that "i", which is uint, can receive it safely. > > In the longer term we might want to move to size_t or even > uintmax_t, as the ulong on a platform may not be long enough in > order to express the largest file size the platform can have, but > this patch (1) is good even without such a change, and (2) gives a > good foundation to build on if we want such a change on top. > > Thanks. Will queue.
This is sad. There is no "may not be long enough". We already know a platform where unsigned long is not long enough, don't we? Why leave this patch in this intermediate state?
If you want to work on data in memory, then size_t is the appropriate data type. We already use it elsewhere. Let's use it here, too, without the intermediate bump from the incorrect `int` to the equally incorrect `long`.
Ciao, Johannes