On Tue, Jun 29, 2021 at 08:11:06AM +0000, Eric Wong wrote:

This benefits unprivileged users who lack permissions to raise
the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource
limit.

Multi-threaded callers to map_loose_object_1 (e.g. "git grep")
appear to guard against thread-safety problems.  Other
multi-core aware pieces of code (e.g. parallel-checkout) uses
child processes

This one makes me slightly more nervous than the last, because we don't
know if somebody higher up the call stack might be expecting to use that
window again.

I _think_ this one is OK, because we would never read a loose object in
the process of reading a packed one. I thought we might in some rare
cases with corruption (e.g., B is a delta against A; we fail to read A
because it's corrupt, so we look elsewhere for a copy). I don't think
the current code is quite that clever, though. If B were corrupted, we'd
look elsewhere for a duplicate, but that logic doesn't apply in the
middle of a delta resolution (even though there are corruption cases it
would help recover from).

So I don't think this is introducing any bugs. But it worries me a bit
that it creates an opportunity for a subtle and hard-to-trigger
situation if we do change seemingly-unrelated code.

But looking more carefully at unuse_one_window(), I think the worst case
is a slight performance drop, as we unmap the window and then re-map
when somebody higher up the call-stack needs it again. My real worry was
that we could trigger a case where somebody was holding onto a pointer
to the bytes. But I think use_pack() will mark those bytes via
inuse_cnt, and then unuse_one_window() will never drop a window that has
a non-zero inuse_count.

So in that sense all of your patches should be correct without thinking
too hard no them. Sure, a performance drop from having to re-map the
window later isn't great, but if the alternative in each case is calling
die(), it's a strict improvement.

-Peff