Re: [PATCH setup 00/14] Use libsolv, solve all our problems... (WIP)
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.
Ken
From f3b3c60ed473a1ef4e5b1ae5fcd1bfc46a6210fb Mon Sep 17 00:00:00 2001
From: Ken Brown
Date: Tue, 17 Oct 2017 08:12:48 -0400
Subject: [PATCH] Avoid dereferencing NULL pointers
The libsolv function pool_id2solvable unconditionally dereferences its
first argument ('pool'). Callers must check that this argument is
non-NULL to avoid crashes.
---
libsolv.cc | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/libsolv.cc b/libsolv.cc
index 78e73a8..3a244d4 100644
--- a/libsolv.cc
+++ b/libsolv.cc
@@ -75,6 +75,8 @@ RelId2Operator(Id id)
const std::string
SolvableVersion::Name () const
{
+ if (!pool)
+return "";
Solvable *solvable = pool_id2solvable(pool, id);
return std::string(pool_id2str(pool, solvable->name));
}
@@ -82,6 +84,8 @@ SolvableVersion::Name () const
const std::string
SolvableVersion::Canonical_version() const
{
+ if (!pool)
+return "";
Solvable *solvable = pool_id2solvable(pool, id);
return std::string(pool_id2str(pool, solvable->evr));
}
@@ -89,6 +93,8 @@ SolvableVersion::Canonical_version() const
package_type_t
SolvableVersion::Type () const
{
+ if (!pool)
+return package_binary;
Solvable *solvable = pool_id2solvable(pool, id);
if (solvable->arch == ARCH_SRC)
return package_source;
@@ -112,6 +118,9 @@ SolvableVersion::obsoletes() const
const PackageDepends
SolvableVersion::deplist(Id keyname) const
{
+ static PackageDepends empty_package;
+ if (!pool)
+return empty_package;
Solvable *solvable = pool_id2solvable(pool, id);
Queue q;
@@ -147,13 +156,14 @@ SolvableVersion::deplist(Id keyname) const
}
// otherwise, return an empty depends list
- static PackageDepends empty_package;
return empty_package;
}
const std::string
SolvableVersion::SDesc () const
{
+ if (!pool)
+return "";
Solvable *solvable = pool_id2solvable(pool, id);
const char *sdesc = repo_lookup_str(solvable->repo, id, SOLVABLE_SUMMARY);
return sdesc;
@@ -197,6 +207,8 @@ SolvableVersion::sourcePackage () const
void
SolvableVersion::fixup_spkg_id (SolvableVersion spkg_id) const
{
+ if (!pool)
+return;
Solvable *solvable = pool_id2solvable(pool, id);
Repodata *data = repo_last_repodata(solvable->repo);
Id handle = id;
@@ -237,6 +249,8 @@ SolvableVersion::accessible () const
package_stability_t
SolvableVersion::Stability () const
{
+ if (!pool)
+return TRUST_UNKNOWN;
Solvable *solvable = pool_id2solvable(pool, id);
Id stability_attr = pool_str2id(pool, "solvable:stability", 1);
return (package_stability_t)repo_lookup_num(solvable->repo, id,
stability_attr, TRUST_UNKNOWN);
--
2.14.2
Re: [PATCH] Revert "Don't override a Keep selection"
On 16/10/2017 20:13, Ken Brown wrote: This reverts (the rest of) commit b43b697. Part of that commit was already reverted in commit ff0bb3d. The rest is not needed either since we no longer send the upgrade flag to the solver after the user has made their selections. --- libsolv.cc | 14 +++--- libsolv.h | 1 - package_meta.h | 2 -- 3 files changed, 3 insertions(+), 14 deletions(-) Hmm... not sure about this. Say the initial upgrade solution had something like: package A 1.0 -> 1.1, and A 1.1 has a dependency on package B 2.0, where currently B 1.0 is installed (so B 1.0 -> 2.0) If the user then changes B to 'keep' (at 1.0), we should report a conflict? Does keeping this cause a problem? I guess we are generating a huge number of these tasks into the solver because "Keep" is kind of overloaded between "Don't care (but it just happens that I know that there's nothing to do)" and "Lock"?
Re: [PATCH setup 00/14] Use libsolv, solve all our problems... (WIP)
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.
Re: [PATCH] Revert "Don't override a Keep selection"
On 10/17/2017 2:43 PM, Jon Turney wrote: On 16/10/2017 20:13, Ken Brown wrote: This reverts (the rest of) commit b43b697. Part of that commit was already reverted in commit ff0bb3d. The rest is not needed either since we no longer send the upgrade flag to the solver after the user has made their selections. --- libsolv.cc | 14 +++--- libsolv.h | 1 - package_meta.h | 2 -- 3 files changed, 3 insertions(+), 14 deletions(-) Hmm... not sure about this. Say the initial upgrade solution had something like: package A 1.0 -> 1.1, and A 1.1 has a dependency on package B 2.0, where currently B 1.0 is installed (so B 1.0 -> 2.0) If the user then changes B to 'keep' (at 1.0), we should report a conflict? Good point. I wasn't thinking about dependencies with version relations. Does keeping this cause a problem? No, but it's not actually effective at the moment, because after commit ff0bb3d, package_meta::default_version isn't being set. Maybe it should be set to reflect the initial upgrade solution. I'll play with this some more. I guess we are generating a huge number of these tasks into the solver because "Keep" is kind of overloaded between "Don't care (but it just happens that I know that there's nothing to do)" and "Lock"? Yeah, this needs further thought. Ken
