From: Junio C Hamano <hidden> Date: 2021-02-05 00:01:25
"Derrick Stolee via GitGitGadget" [off-list ref] writes:
From: Derrick Stolee <redacted>
When calculating the sizes of certain chunks, we should use 64-bit
multiplication always. This allows us to properly predict the chunk
sizes without risk of overflow.
Signed-off-by: Derrick Stolee <redacted>
---
midx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
This one I find somewhat questionable for multiple reasons.
* the fourth parameter of add_chunk() is of size_t, not uint64_t;
shouldn't the multiplication be done in type size_t instead?
* these mutiplications were introduced in "midx: use chunk-format
API in write_midx_internal()"; that step should use the
arithmetic with cast (if necessary) from the start, no?
* There is "ctx.entries_nr * MIDX_CHUNKID_OFFSET_WIDTH" passed to
add_chunk(), in the post-context of the first hunk. Shouldn't
that be covered as well? I didn't grep for all uses of
add_chunk(), but I wouldn't be surprised if this patch missed
some of the calls that need the same treatment.
"Derrick Stolee via GitGitGadget" [off-list ref] writes:
quoted
From: Derrick Stolee <redacted>
When calculating the sizes of certain chunks, we should use 64-bit
multiplication always. This allows us to properly predict the chunk
sizes without risk of overflow.
Signed-off-by: Derrick Stolee <redacted>
---
midx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
This one I find somewhat questionable for multiple reasons.
* the fourth parameter of add_chunk() is of size_t, not uint64_t;
shouldn't the multiplication be done in type size_t instead?
This is probably appropriate because we will truncate to size_t if
it is smaller than uint64_t.
* these mutiplications were introduced in "midx: use chunk-format
API in write_midx_internal()"; that step should use the
arithmetic with cast (if necessary) from the start, no?
I wanted to isolate these changes specifically so we could be
careful about the multiplications and not be distracted by them
when converting to the chunk-format API. The multiplications were
"moved" by that patch, not "introduced".
* There is "ctx.entries_nr * MIDX_CHUNKID_OFFSET_WIDTH" passed to
add_chunk(), in the post-context of the first hunk. Shouldn't
that be covered as well? I didn't grep for all uses of
add_chunk(), but I wouldn't be surprised if this patch missed
some of the calls that need the same treatment.
And here is a great example of why it was good to call out these
multiplications in their own patch.
I did a full inspection of all multiplications in midx.c and
found a few more instances of possible overflow. Two are on the
read side, but they require the object lookup chunk to have size
4gb or larger. This is not _that_ far off from possibility! My
multi-pack-index for the Windows repository is currently ~1.6 GB
(in total, including the other chunks).
Thanks,
-Stolee
From: Junio C Hamano <hidden> Date: 2021-02-05 21:18:45
Chris Torek [off-list ref] writes:
On Thu, Feb 4, 2021 at 4:00 PM Junio C Hamano [off-list ref] wrote:
quoted
* the fourth parameter of add_chunk() is of size_t, not uint64_t;
shouldn't the multiplication be done in type size_t instead?
There are (still) systems with 32-bit size_t (but 64-bit
off_t / file sizes), so ... probably not. Is size_t ever more than
64 bits these days?
Sorry, you lost me. I do not see how it would help to perform the
multiplication in uint64_t, when you suspect that size_t is too
small, if the final destination of the result of the multiplication
is a function argument of type size_t?
From: Chris Torek <hidden> Date: 2021-02-06 20:36:37
On Fri, Feb 5, 2021 at 12:41 PM Junio C Hamano [off-list ref] wrote:
Chris Torek [off-list ref] writes:
quoted
There are (still) systems with 32-bit size_t (but 64-bit
off_t / file sizes), so ... probably not. Is size_t ever more than
64 bits these days?
Sorry, you lost me. I do not see how it would help to perform the
multiplication in uint64_t, when you suspect that size_t is too
small, if the final destination of the result of the multiplication
is a function argument of type size_t?
No, you and Derrick Stolee are right, I wasn't looking out far enough
here (to the actual function).
(I was wondering though if there are systems where the valid range
for size_t could exceed that for off_t. Are there still systems
using 32-bit off_t? Sometimes I think there are too many abstracted
types running around here -- how do we know which sizes are big
enough? There is always uintmax_t, though, and for unsigned
types, ((T)-1) gets you the maximum possible value.)
Chris
From: SZEDER Gábor <hidden> Date: 2021-02-07 19:51:48
On Thu, Feb 04, 2021 at 04:00:19PM -0800, Junio C Hamano wrote:
"Derrick Stolee via GitGitGadget" [off-list ref] writes:
quoted
From: Derrick Stolee <redacted>
When calculating the sizes of certain chunks, we should use 64-bit
multiplication always. This allows us to properly predict the chunk
sizes without risk of overflow.
Signed-off-by: Derrick Stolee <redacted>
---
midx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
This one I find somewhat questionable for multiple reasons.
* the fourth parameter of add_chunk() is of size_t, not uint64_t;
shouldn't the multiplication be done in type size_t instead?
* these mutiplications were introduced in "midx: use chunk-format
API in write_midx_internal()";
No, that patch also removes lines like:
- chunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] + ctx.entries_nr * the_hash_algo->rawsz;
- chunk_offsets[cur_chunk] = chunk_offsets[cur_chunk - 1] +
- ctx.num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH;
So those potentially problematic multiplications were already there
before this series, and in fact trace all the way back to the initial
midx patch series (commits 0d5b3a5ef7 (midx: write object ids in a
chunk, 2018-07-12) and 662148c435 (midx: write object offsets,
2018-07-12)).
that step should use the
arithmetic with cast (if necessary) from the start, no?
As it fixes a long-standing issue, it should rather be a bugfix patch
at the beginning of the series.