[PATCH v5 2/4] Add system-wide tunables: cache ld.so.cache

[DJ Delorie]

Yury Khrustalev <yury.khrustalev@arm.com> writes:
>> > This is such an extensive change. Is the existing functionality covered
>> > with any tests?
>> 
>> It's the dynamic loader.  Every program in the testsuite is a test ;-)
>
> Not every program uses all features of ld.so.cache and config though.
> If this patch breaks some corner case that is not covered by any of the
> tests, we would have introduced a regression.
>
> My question was about how good is test coverage of this code with the
> existing tests? I suppose most use one or two most straightforward
> situations which is implemented by a small chunk of this code?

You're right that specific tests are best, I'm just trying to figure out
what (if anything) we can specifically test here that isn't already
tested elsewhere.  Ideally, we'd have a comprehensive test plan that
covers all required functionality and that would cover this, but
typically we only add test cases where we've had bugs in the past (to
avoid regressions).  So our current testing is spotty at best.  This
case (ld.so) is at least better because it's such a core part of every
program that most bugs show up quickly even without test cases.

>> The whole point of caching the data is to, well, cache the data.  It's
>> not freed unless it's being replaced by data from a known-good cache
>> file.  This was requested by Carlos, so I'll defer to him to justify it.
>> IIRC at least part of the reason was to ensure that dlopen() had valid
>> cache data to work with, even if the file is corrupted while the program
>> was running.
>
> Hmm, sorry, I don't follow. Previously memory was unmapped when this
> function was called and cache was reset and now none of that happens.
>
> I'm not asking why we needed to clear cache from some places, I'm saying
> that now we no longer do this. If it's supposed to somehow happen behind
> the scenes, then we should just remove calls to this function and leave
> a comment explaining what actually happens to unloading cache.

/me tries to recall Carlos's justification...

  Consider: you run a program, it loads it's SOs, starts running.  While
  it's running, the admin edits ld.so.conf and manages to corrupt the
  cache.  The program then goes to install some add-on and dlopen()'s
  another SO.  The cache is corrupt and the program crashes.  Better:
  the cache is corrupt, so we use the previously loaded cache instead.

Given that, it implies we keep that mapping around while the program is
running.

As for why I did the patch the way I did... honestly, it was so long ago
I really don't recall.  I suppose I thought that part might not get
consensus and have to be reverted, in which case "change one spot" is
better.  Or I thought it would be easier to see what the change was that
way.  Or I didn't want to preclude some other "I'm done with the cache,
it's safe to XYZ" functionality in the future, which would need those
calls to close-but-not-unmap the cache.

So the API still has pairs of calls - "I need the cache" and "I'm done
with the cache", it's just that the implementation need not do anything
for the second call.