On 10/17/2017 2:46 PM, Jon Turney wrote:
On 17/10/2017 13:44, Ken Brown wrote:
On 10/10/2017 7:18 AM, Ken Brown wrote:
On 9/29/2017 4:33 PM, Ken Brown wrote:
I'll resume my testing after I return.
I've just started testing (based on the current HEAD of
topic/libsolv), and so far everything looks good.
I came across a situation where a SolvableVersion method was being
called on a trivial object (with pool and id both 0). This caused a
crash when pool_id2solvable(pool, id) was called and pool was
dereferenced. There's probably a bug that led to this situation. [It
involved a local install in which a package was listed in two
different setup.ini files, but the tarballs existed only in one.] I
plan to investigate this further. But in any case, we shouldn't
crash. Patch attached.
I thought about putting this in, but decided against it as it would
probably catch some mistakes I had made...
But yeah, for production use, I think not crashing is probably a good
idea :). Although I guess we might want asserts or something, if we
think these cases shouldn't be happening.
Here's the situation where I got the crash: Package A is installed and
requires B. setup tries to install B, but the install fails for some
reason. During the postinstall phase, we're scanning the dependencies
of A and we call checkForInstalled to see if B is installed. This ends
up calling PackageSpecification::satisfies(aPackage), with aPackage
being the empty package (pool == NULL, id == 0) because B is not
installed. A call to aPackage.Name() then causes the crash.
I think this sequence of calls is reasonable. Name() is part of the
public interface to SolvableVersion, so we should be able to call Name()
on any package without a crash or assertion violation. In the case of
the empty package, the empty string is a reasonable return value.
Similar considerations apply to the other public member functions of
SolvableVersion. So my inclination is to go with something like my
patch rather than changing all callers.
Ken
