Subject: apk_blob_pull_dep corrupts virtpkg->version->ptr under clang

From: Chloe Kudryavtsev <[email-masked]>
Date: Wed, 10 Jul 2019 19:55:47 -0400

Hi, virtual packages get their version corrupted under clang.
It specifically happens in add.c::171 (well, the invocation, digging 
deeper turned out to be difficult).

Steps to reproduce:
1. Launch a docker image - I used alpine:edge
2. Install the packages we'll need for the demonstration, full list at [1].
3. git clone https://git.alpinelinux.org/apk-tools
4. cd apk-tools
5. Apply the following patch: https://brpaste.xyz/raw/2B_m0A. Reasoning 
in [2]. (curl https://brpaste.xyz/raw/2B_m0A | patch -p1)
6. make CC=clang CFLAGS=-O2
7. ./src/apk -s add -t foo foo
8. Observe the discrepancies

NOTE: CFLAGS are -O2 because that makes the error easier to observe.
However it shows up even under -O0.

I have no familiarity with apk internals, but clang compiles many other 
things just fine without corrupting pointers at random.
Could someone look into this?

[1]: musl-dev linux-headers lua5.2-dev openssl-dev zlib-dev clang gcc 
make git curl
[2]: This adds two printf statements for easy viewing and removes -Wall 
(clang has stricter warnings, should be tackled, but maybe later).

---

From: "A. Wilcox" <[email-masked]>
Date: Wed, 10 Jul 2019 19:45:57 -0500

> (clang has stricter warnings, should be tackled, but maybe later).

Duplicated on Adélie ppc64 with clang 8.

This is because 'ver' is a stack-allocated variable.  Its scope is
bounded by create_virtual_package.

Patch attached fixes this.

--arw

-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org
From d08073794a71c5bf2ad1df52799d359f55da0680 Mon Sep 17 00:00:00 2001
From: "A. Wilcox" <[email-masked]>
Date: Wed, 10 Jul 2019 19:44:48 -0500
Subject: [PATCH] add: create_virtual_package: dup ver string

ver is a stack-allocated variable.  Its scope ends when the function does.
This means that the version atom is no longer valid after the return of
create_virtual_package.
---
 src/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/add.c b/src/add.c
index 4c285f7..7e15f10 100644
--- a/src/add.c
+++ b/src/add.c
@@ -97,7 +97,7 @@ static struct apk_package *create_virtual_package(struct apk_database *db, struc
 	if (virtpkg == NULL) return 0;
 
 	virtpkg->name = name;
-	virtpkg->version = apk_blob_atomize(APK_BLOB_STR(ver));
+	virtpkg->version = apk_blob_atomize_dup(APK_BLOB_STR(ver));
 	virtpkg->description = strdup("virtual meta package");
 	virtpkg->arch = apk_blob_atomize(APK_BLOB_STR("noarch"));
 
-- 
2.22.0

---

From: "A. Wilcox" <[email-masked]>
Date: Wed, 10 Jul 2019 20:26:00 -0500

> (clang has stricter warnings, should be tackled, but maybe later).

My previous fix for this was incomplete.  I ran APK through Valgrind
afterwards and found that it was insufficient; it was duplicating the
atom but not the blob string itself, so there was still an invalid read.

v2 of this patch handles that, strdup()ing the version string the same
as the description string.  Additionally, Valgrind leads me to believe
that the checksum data may also being allocated on the stack:

==56753== Use of uninitialised value of size 8
==56753==    at 0x12F204: apk_hash_get_hashed (hash.c:0)
==56753==    by 0x11B1DF: apk_hash_get (apk_hash.h:70)
==56753==    by 0x11BAE7: apk_db_pkg_add (database.c:528)
==56753==    by 0x112803: add_main (add.c:193)
==56753==    by 0x1115EB: main (apk.c:681)
==56753==  Uninitialised value was created by a stack allocation
==56753==    at 0x112B24: create_virtual_package (add.c:85)
==56753==
==56753== Conditional jump or move depends on uninitialised value(s)
==56753==    at 0x4913260: memcmp (in
/usr/lib/valgrind/vgpreload_memcheck-ppc64be-linux.so)
==56753==    by 0x12DFEB: apk_blob_compare (blob.c:262)
==56753==    by 0x12F2C7: apk_hash_get_hashed (hash.c:66)
==56753==    by 0x11B1DF: apk_hash_get (apk_hash.h:70)
==56753==    by 0x11BAE7: apk_db_pkg_add (database.c:528)
==56753==    by 0x112803: add_main (add.c:193)
==56753==    by 0x1115EB: main (apk.c:681)
==56753==  Uninitialised value was created by a stack allocation
==56753==    at 0x112B24: create_virtual_package (add.c:85)

Patch v2 does not rectify this.  I am still investigating.

Best,
--arw

From f865ed5ab0667bbf5fdf4e0da3b09b4eba189567 Mon Sep 17 00:00:00 2001

diff --git a/src/add.c b/src/add.c
index 4c285f7..62fde4a 100644
--- a/src/add.c
+++ b/src/add.c
@@ -97,7 +97,7 @@ static struct apk_package *create_virtual_package(struct apk_database *db, struc
 	if (virtpkg == NULL) return 0;
 
 	virtpkg->name = name;
-	virtpkg->version = apk_blob_atomize(APK_BLOB_STR(ver));
+	virtpkg->version = apk_blob_atomize(APK_BLOB_STR(strdup(ver)));
 	virtpkg->description = strdup("virtual meta package");
 	virtpkg->arch = apk_blob_atomize(APK_BLOB_STR("noarch"));
 
-- 
2.22.0