From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Martin Koegler <martin.koegler@chello.at>
Subject: Re: [PATCH] zlib.c: use size_t for size
Date: Fri, 12 Oct 2018 22:46:24 -0400 [thread overview]
Message-ID: <20181013024624.GB15595@sigill.intra.peff.net> (raw)
In-Reply-To: <20181013023845.GA15595@sigill.intra.peff.net>
On Fri, Oct 12, 2018 at 10:38:45PM -0400, Jeff King wrote:
> So right now let's imagine that off_t is 64-bit, and "unsigned long" is
> 32-bit (e.g., 32-bit system, or an IL32P64 model like Windows). We'll
> repeatedly ask use_pack() for a window, and it will tell us how many
> bytes we have in "avail". So even as a 32-bit value, that just means
> we'll process chunks smaller than 4GB, and this is correct (or at least
> this part of it -- hold on). But we can still process the whole "len"
> given by the off_t eventually.
So this "hold on" was because I thought I had found another bug in
use_pack(), but I actually think it's OK.
In use_pack(), we do this:
if (left)
*left = win->len - xsize_t(offset);
where win->len is a size_t. Before this patch, "left" is a pointer to
unsigned long. So that has the usual broken-on-Windows mismatch. This
patch makes it a size_t, which is good.
But what's up with that xsize_t(offset)? We'll complain about truncation
_before_ we do any offset subtraction. So at first glance, I thought
this meant we were broken for larger-than-4GB packs on 32-bit systems
when trying to read past the 4GB mark.
But no, right before that we have this line:
offset -= win->offset;
So offset is in fact no longer its original meaning of "offset into the
packfile", but is now an offset of the specific request into the window
we found.
So I think it's correct, but it sure confused me. I wonder if another
variable might help, like:
size_t offset_in_window;
...
/*
* We know this difference will fit in a size_t, because our mmap
* window by * definition can be no larger than a size_t.
*/
offset_in_window = xsize_t(offset - win->offset);
if (left)
*left = win->len - offset_in_window;
return win->base + offset_in_window;
I dunno. Maybe it is overkill.
-Peff
next prev parent reply other threads:[~2018-10-13 2:46 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-12 7:07 [PATCH] zlib.c: use size_t for size Junio C Hamano
2018-10-12 9:54 ` Johannes Schindelin
2018-10-12 13:52 ` Junio C Hamano
2018-10-12 15:34 ` Johannes Schindelin
2018-10-12 23:23 ` Ramsay Jones
2018-10-12 20:42 ` [PATCH v2 1/1] " tboegi
2018-10-12 22:22 ` SZEDER Gábor
2018-10-13 5:00 ` Torsten Bögershausen
2018-10-14 2:16 ` Ramsay Jones
2018-10-14 2:31 ` Ramsay Jones
2018-10-14 2:52 ` Jeff King
2018-10-14 15:03 ` Ramsay Jones
2018-10-15 0:01 ` Jeff King
2018-10-15 0:41 ` Ramsay Jones
2018-10-15 4:22 ` Junio C Hamano
2018-10-15 5:54 ` Torsten Bögershausen
2018-10-13 2:38 ` [PATCH] " Jeff King
2018-10-13 2:46 ` Jeff King [this message]
2018-10-13 8:43 ` Johannes Sixt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181013024624.GB15595@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=martin.koegler@chello.at \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).