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).

On 07/10/19 18:55, Chloe Kudryavtsev wrote:
> 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).



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

On 07/10/19 18:55, Chloe Kudryavtsev wrote:
> 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).


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


-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org