[PATCH userspace 0/2] Bump testsuite CI image to F34

[PATCH userspace 0/2] Bump testsuite CI image to F34

[Ondrej Mosnacek]

Run the testsuite on Fedora 34 to both keep with the flow and make the
vsock_socket test pass. Also fix an invalid free bug found by GCC 11
(needed for the build to succeed on F34).

The GitHub Actions CI now passes as verified on my GH fork.

Ondrej Mosnacek (2):
  libselinux: fix invalid free in store_stem()/load_mmap()
  scripts/ci: use F34 image instead of F33

 libselinux/src/label_file.c |  3 +--
 libselinux/src/label_file.h | 12 +++++++-----
 scripts/ci/Vagrantfile      |  2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.31.1

[PATCH userspace 1/2] libselinux: fix invalid free in store_stem()/load_mmap()

[Ondrej Mosnacek]

Building libselinux with GCC 11.1.1 produces the following warning:
```
In file included from label_file.c:24:
In function ‘store_stem’,
    inlined from ‘load_mmap’ at label_file.c:277:12,
    inlined from ‘process_file’ at label_file.c:551:5:
label_file.h:289:25: error: ‘free’ called on pointer ‘*mmap_area.next_addr’ with nonzero offset 4 [-Werror=free-nonheap-object]
  289 |                         free(buf);
      |                         ^~~~~~~~~
```

Indeed, in this case the pointer shouldn't be freed as it comes from
mmap. Fix this by adding a from_mmap parameter to store_stem() instead
of overriding the saved_data::from_mmap value in load_mmap().

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 libselinux/src/label_file.c |  3 +--
 libselinux/src/label_file.h | 12 +++++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index cfce23e0..199ae66b 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -274,12 +274,11 @@ end_arch_check:
 		/* store the mapping between old and new */
 		newid = find_stem(data, buf, stem_len);
 		if (newid < 0) {
-			newid = store_stem(data, buf, stem_len);
+			newid = store_stem(data, buf, stem_len, true);
 			if (newid < 0) {
 				rc = newid;
 				goto out;
 			}
-			data->stem_arr[newid].from_mmap = 1;
 		}
 		stem_map[i] = newid;
 	}
diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index baed3341..66e5721f 100644
--- a/libselinux/src/label_file.h
+++ b/libselinux/src/label_file.h
@@ -62,7 +62,7 @@ struct spec {
 struct stem {
 	char *buf;
 	int len;
-	char from_mmap;
+	bool from_mmap;
 };
 
 /* Where we map the file in during selabel_open() */
@@ -276,7 +276,8 @@ static inline int find_stem(struct saved_data *data, const char *buf,
 }
 
 /* returns the index of the new stored object */
-static inline int store_stem(struct saved_data *data, char *buf, int stem_len)
+static inline int store_stem(struct saved_data *data, char *buf, int stem_len,
+			     bool from_mmap)
 {
 	int num = data->num_stems;
 
@@ -286,7 +287,8 @@ static inline int store_stem(struct saved_data *data, char *buf, int stem_len)
 		tmp_arr = realloc(data->stem_arr,
 				  sizeof(*tmp_arr) * alloc_stems);
 		if (!tmp_arr) {
-			free(buf);
+			if (!from_mmap)
+				free(buf);
 			return -1;
 		}
 		data->alloc_stems = alloc_stems;
@@ -294,7 +296,7 @@ static inline int store_stem(struct saved_data *data, char *buf, int stem_len)
 	}
 	data->stem_arr[num].len = stem_len;
 	data->stem_arr[num].buf = buf;
-	data->stem_arr[num].from_mmap = 0;
+	data->stem_arr[num].from_mmap = from_mmap;
 	data->num_stems++;
 
 	return num;
@@ -321,7 +323,7 @@ static inline int find_stem_from_spec(struct saved_data *data, const char *buf)
 	if (!stem)
 		return -1;
 
-	return store_stem(data, stem, stem_len);
+	return store_stem(data, stem, stem_len, false);
 }
 
 /* This will always check for buffer over-runs and either read the next entry
-- 
2.31.1

[PATCH userspace 2/2] scripts/ci: use F34 image instead of F33

[Ondrej Mosnacek]

Now that F34 has been released, it's time to update the CI Vagrantfile
to use the new Fedora version. This also fixes the failure in the
recently added vsock_socket test that depends on a bugfix, which made it
to the F34 image's kernel, but is not in the F33 image's.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 scripts/ci/Vagrantfile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/ci/Vagrantfile b/scripts/ci/Vagrantfile
index d7c7bb39..20c523a0 100644
--- a/scripts/ci/Vagrantfile
+++ b/scripts/ci/Vagrantfile
@@ -34,7 +34,7 @@ SCRIPT
 # backwards compatibility). Please don't change it unless you know what
 # you're doing.
 Vagrant.configure("2") do |config|
-  config.vm.box = "fedora/33-cloud-base"
+  config.vm.box = "fedora/34-cloud-base"
   config.vm.synced_folder "../..", "/root/selinux"
 
   config.vm.provider "virtualbox" do |v|
-- 
2.31.1

Re: [PATCH userspace 1/2] libselinux: fix invalid free in store_stem()/load_mmap()

[Christian Göttsche]

Am Mi., 12. Mai 2021 um 12:25 Uhr schrieb Ondrej Mosnacek <omosnace@redhat.com>:
>
> Building libselinux with GCC 11.1.1 produces the following warning:
> ```
> In file included from label_file.c:24:
> In function ‘store_stem’,
>     inlined from ‘load_mmap’ at label_file.c:277:12,
>     inlined from ‘process_file’ at label_file.c:551:5:
> label_file.h:289:25: error: ‘free’ called on pointer ‘*mmap_area.next_addr’ with nonzero offset 4 [-Werror=free-nonheap-object]
>   289 |                         free(buf);
>       |                         ^~~~~~~~~
> ```
>
> Indeed, in this case the pointer shouldn't be freed as it comes from
> mmap. Fix this by adding a from_mmap parameter to store_stem() instead
> of overriding the saved_data::from_mmap value in load_mmap().
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

See https://patchwork.kernel.org/project/selinux/patch/20210503175350.55954-17-cgzones@googlemail.com/
for an alternative.

Re: [PATCH userspace 2/2] scripts/ci: use F34 image instead of F33

[Petr Lautrbach]

Ondrej Mosnacek <omosnace@redhat.com> writes:

> Now that F34 has been released, it's time to update the CI Vagrantfile
> to use the new Fedora version. This also fixes the failure in the
> recently added vsock_socket test that depends on a bugfix, which made it
> to the F34 image's kernel, but is not in the F33 image's.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Acked-by: Petr Lautrbach <plautrba@redhat.com>


> ---
>  scripts/ci/Vagrantfile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/ci/Vagrantfile b/scripts/ci/Vagrantfile
> index d7c7bb39..20c523a0 100644
> --- a/scripts/ci/Vagrantfile
> +++ b/scripts/ci/Vagrantfile
> @@ -34,7 +34,7 @@ SCRIPT
>  # backwards compatibility). Please don't change it unless you know what
>  # you're doing.
>  Vagrant.configure("2") do |config|
> -  config.vm.box = "fedora/33-cloud-base"
> +  config.vm.box = "fedora/34-cloud-base"
>    config.vm.synced_folder "../..", "/root/selinux"
>  
>    config.vm.provider "virtualbox" do |v|
> -- 
> 2.31.1

Re: [PATCH userspace 2/2] scripts/ci: use F34 image instead of F33

[Petr Lautrbach]

Petr Lautrbach <plautrba@redhat.com> writes:

> Ondrej Mosnacek <omosnace@redhat.com> writes:
>
>> Now that F34 has been released, it's time to update the CI Vagrantfile
>> to use the new Fedora version. This also fixes the failure in the
>> recently added vsock_socket test that depends on a bugfix, which made it
>> to the F34 image's kernel, but is not in the F33 image's.
>>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> Acked-by: Petr Lautrbach <plautrba@redhat.com>

Merged

>
>> ---
>>  scripts/ci/Vagrantfile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/ci/Vagrantfile b/scripts/ci/Vagrantfile
>> index d7c7bb39..20c523a0 100644
>> --- a/scripts/ci/Vagrantfile
>> +++ b/scripts/ci/Vagrantfile
>> @@ -34,7 +34,7 @@ SCRIPT
>>  # backwards compatibility). Please don't change it unless you know what
>>  # you're doing.
>>  Vagrant.configure("2") do |config|
>> -  config.vm.box = "fedora/33-cloud-base"
>> +  config.vm.box = "fedora/34-cloud-base"
>>    config.vm.synced_folder "../..", "/root/selinux"
>>  
>>    config.vm.provider "virtualbox" do |v|
>> -- 
>> 2.31.1

Re: [PATCH userspace 1/2] libselinux: fix invalid free in store_stem()/load_mmap()

[Petr Lautrbach]

Christian Göttsche <cgzones@googlemail.com> writes:

> Am Mi., 12. Mai 2021 um 12:25 Uhr schrieb Ondrej Mosnacek <omosnace@redhat.com>:
>>
>> Building libselinux with GCC 11.1.1 produces the following warning:
>> ```
>> In file included from label_file.c:24:
>> In function ‘store_stem’,
>>     inlined from ‘load_mmap’ at label_file.c:277:12,
>>     inlined from ‘process_file’ at label_file.c:551:5:
>> label_file.h:289:25: error: ‘free’ called on pointer ‘*mmap_area.next_addr’ with nonzero offset 4 [-Werror=free-nonheap-object]
>>   289 |                         free(buf);
>>       |                         ^~~~~~~~~
>> ```
>>
>> Indeed, in this case the pointer shouldn't be freed as it comes from
>> mmap. Fix this by adding a from_mmap parameter to store_stem() instead
>> of overriding the saved_data::from_mmap value in load_mmap().
>>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> See https://patchwork.kernel.org/project/selinux/patch/20210503175350.55954-17-cgzones@googlemail.com/
> for an alternative.

https://patchwork.kernel.org/project/selinux/patch/20210503175350.55954-17-cgzones@googlemail.com/ - Accepted

https://patchwork.kernel.org/project/selinux/patch/20210512102529.122753-2-omosnace@redhat.com/ - Rejected

Thanks to both!