On Thu, Jan 11, 2024 at 10:18:58PM +0100, Emanuele Rocca wrote:
> Hi Julian,
>
> On 2024-01-11 05:46, Julian Andres Klode wrote:
> > And there aren't any hard errors. We could zero initialize
> > those or add supressions to make things look nicer I suppose.
>
> Mmmh no, they are all actual errors as far as valgrind is concerned.
>
> The thing is, you're running valgrind without --error-exitcode. By doing
> so, the exit code of your tests is the exit code of apt-get, not of
> valgrind.
>
> Try this instead:
>
> (sid-amd64)root@ariel:~# valgrind --error-exitcode=1 apt-get update
> [...]
> ==308534== ERROR SUMMARY: 6 errors from 6 contexts (suppressed: 0 from 0)
> (sid-amd64)root@ariel:~# echo $?
> 1
That's all just strawman arguments.
As said before, these aren't the issue. These all occur in locations
where we read the entire memory arena of our allocator, either for
hashing or writing it to a file.
While they _should_ be zero-initialized if you look at the code, we
actually initialize things with a header object. The problem is the
header object looks like:
uint32_t magic;
uint16_t major;
uint16_t minor;
bool dirty;
uint16_t boo;
And the padding bytes are never initialized but will inevitably
be read by the hashing and write back of the entire arena.
We can fix these errors by either zero initializing the padding in
the object constructor:
diff --git a/apt-pkg/pkgcache.cc b/apt-pkg/pkgcache.cc
index 76336b9b3..b845cb13c 100644
--- a/apt-pkg/pkgcache.cc
+++ b/apt-pkg/pkgcache.cc
@@ -52,6 +52,7 @@ using APT::StringView;
/* Simply initialize the header */
pkgCache::Header::Header()
{
+ memset(this, 0, sizeof(*this));
#define APT_HEADER_SET(X,Y) X = Y;
static_assert(std::numeric_limits<decltype(X)>::max() > Y, "Size violation
detected in pkgCache::Header")
APT_HEADER_SET(Signature, 0x98FE76DC);
or by not creating a header on the stack (with uninitialized padding) and then
copying
it in, by using placement new operator:
diff --git a/apt-pkg/pkgcachegen.cc b/apt-pkg/pkgcachegen.cc
index 9e47ef369..3a85a9585 100644
--- a/apt-pkg/pkgcachegen.cc
+++ b/apt-pkg/pkgcachegen.cc
@@ -89,7 +89,7 @@ bool pkgCacheGenerator::Start()
Map.UsePools(*Cache.HeaderP->Pools,sizeof(Cache.HeaderP->Pools)/sizeof(Cache.HeaderP->Pools[0]));
// Starting header
- *Cache.HeaderP = pkgCache::Header();
+ new (Cache.HeaderP) pkgCache::Header();
// make room for the hashtables for packages and groups
if (Map.RawAllocate(2 * (Cache.HeaderP->GetHashTableSize() *
sizeof(map_pointer<void>))) == 0)
Either way, these are harmless
--
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer i speak de, en
