From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-2.7 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,RCVD_IN_SORBS_SPAM, RP_MATCHES_RCVD shortcircuit=no autolearn=no autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 434C7208B8 for ; Fri, 11 Aug 2017 06:58:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751492AbdHKG6B (ORCPT ); Fri, 11 Aug 2017 02:58:01 -0400 Received: from vie01a-dmta-pe02-3.mx.upcmail.net ([62.179.121.159]:41838 "EHLO vie01a-dmta-pe02-3.mx.upcmail.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751299AbdHKG6A (ORCPT ); Fri, 11 Aug 2017 02:58:00 -0400 Received: from [172.31.216.44] (helo=vie01a-pemc-psmtp-pe02) by vie01a-dmta-pe02.mx.upcmail.net with esmtp (Exim 4.88) (envelope-from ) id 1dg3t9-0004kx-HD for git@vger.kernel.org; Fri, 11 Aug 2017 08:57:43 +0200 Received: from master.zuhause ([80.108.242.240]) by vie01a-pemc-psmtp-pe02 with SMTP @ mailcloud.upcmail.net id vixY1v0215BuuEg01ixZ3q; Fri, 11 Aug 2017 08:57:34 +0200 X-SourceIP: 80.108.242.240 Received: by master.zuhause (Postfix, from userid 1006) id 8B2F445D4512; Fri, 11 Aug 2017 08:57:32 +0200 (CEST) Date: Fri, 11 Aug 2017 08:57:32 +0200 From: Martin Koegler To: Junio C Hamano Cc: Martin Koegler , git@vger.kernel.org, Johannes.Schindelin@gmx.de Subject: Re: [PATCH 4/4] Fix delta offset overflow Message-ID: <20170811065732.GA15128@mail.zuhause> References: <1502388789-5775-1-git-send-email-martin@mail.zuhause> <1502388789-5775-2-git-send-email-martin@mail.zuhause> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Thu, Aug 10, 2017 at 01:49:24PM -0700, Junio C Hamano wrote: > The lower 4-byte of moff (before incrementing it with msize) were > already encoded to the output stream before this hunk. Shouldn't > we be checking if moff would fit in uint32_t _before_ that happens? moff is otherwise only decremented or assigned with an offset generated by create_delta_index. These offsets are limited by 4GB. Any larger offets would be a programming bug - so qualify for just a "assert". > IOW, in a much earlier part of that "while (data < top)" loop, there > is a code that tries to find a match that gives us a large msize by > iterating over index->hash[], and it sets msize and moff as a potential > location that we may want to use. If moff is already big there, then > we shouldn't consider it a match because we cannot encode its location > using 4-byte anyway, no? Recovering from an incorrect moff would add more complex code for a case, that should not happen. A bailout would be sufficient. > Cutting it off at here by resetting msize to 0 might help the next > iteration (I didn't check, but is the effect of it is to corrupt the > "val" rolling checksum and make it unlikely that the hash > computation would not find a correct match?) but it somehow feels > like closing the barn door after the horse has already bolted... The current code produces incorrect deltas - its not just a checksum issue. By the way: Somebody interested in JGIT should also look at these two bugs: https://github.com/eclipse/jgit/blob/005e5feb4ecd08c4e4d141a38b9e7942accb3212/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/DeltaEncoder.java copy would also encode beyond 4GB - producing truncated delta offset. https://github.com/eclipse/jgit/blob/005e5feb4ecd08c4e4d141a38b9e7942accb3212/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/BinaryDelta.java apply uses int for decoding length values. Regards, Martin