[PATCH] pkg_parse: don't segfault on bogus Size/Installed-Size

[Brenda Streiff]

Refactor the parsing of integer-valued fields into its own function,
and verify that the string is non-empty and a positive unsigned long
integer. (Previously, an empty string would result in a segfault.)

Fixes bugzilla: 14178

Signed-off-by: Brenda Streiff <brenda....@ni.com>
---
libopkg/pkg_parse.c | 48 ++++++++++++++++-------
tests/Makefile | 1 +
tests/regress/issue14178.py | 77 +++++++++++++++++++++++++++++++++++++
3 files changed, 112 insertions(+), 14 deletions(-)
create mode 100755 tests/regress/issue14178.py

diff --git a/libopkg/pkg_parse.c b/libopkg/pkg_parse.c
index 2213b88..582db66 100644
--- a/libopkg/pkg_parse.c
+++ b/libopkg/pkg_parse.c
@@ -64,6 +64,32 @@ static void parse_conffiles(pkg_t * pkg, const char *cstr)
conffile_list_append(&pkg->conffiles, file_name, md5sum);
}

+static unsigned long parse_ulong(pkg_t *pkg, const char *field,
+ const char *line)
+{
+ unsigned long value = 0;
+ char *tmp, *endptr;
+
+ tmp = parse_simple(field, line);
+ if (!tmp) {
+ opkg_msg(ERROR, "Failed to parse %s line for %s\n", field, pkg->name);
+ return value;
+ }
+
+ errno = 0;
+ endptr = NULL;
+ value = strtoul(tmp, &endptr, 0);
+ /* strtoul won't reject negative values, so check the first character
+ for a minus sign. The string returned by parse_simple has leading
+ whitespace removed, so *tmp should be the first non-whitespace character
+ */
+ if (errno || (*endptr != '\0') || (*tmp == '-'))
+ opkg_msg(ERROR, "Failed to parse %s line for %s\n", field, pkg->name);
+
+ free(tmp);
+ return value;
+}
+
static void parse_userfields(pkg_t *pkg, const char *cstr)
{
char name[1024], value[4096];
@@ -200,15 +226,11 @@ int pkg_parse_line(void *ptr, const char *line, uint mask)
break;

case 'I':
- if ((mask & PFM_INSTALLED_SIZE) && is_field("Installed-Size", line)) {
- char *tmp = parse_simple("Installed-Size", line);
- pkg->installed_size = strtoul(tmp, NULL, 0);
- free(tmp);
- } else if ((mask & PFM_INSTALLED_TIME) && is_field("Installed-Time", line)) {
- char *tmp = parse_simple("Installed-Time", line);
- pkg->installed_time = strtoul(tmp, NULL, 0);
- free(tmp);
- } else if (opkg_config->verbose_status_file)
+ if ((mask & PFM_INSTALLED_SIZE) && is_field("Installed-Size", line))
+ pkg->installed_size = parse_ulong(pkg, "Installed-Size", line);
+ else if ((mask & PFM_INSTALLED_TIME) && is_field("Installed-Time", line))
+ pkg->installed_time = parse_ulong(pkg, "Installed-Time", line);
+ else if (opkg_config->verbose_status_file)
userfield = 1;
break;

@@ -254,11 +276,9 @@ int pkg_parse_line(void *ptr, const char *line, uint mask)
pkg->section = parse_simple("Section", line);
else if ((mask & PFM_SHA256SUM) && is_field("SHA256sum", line))
pkg->sha256sum = parse_simple("SHA256sum", line);
- else if ((mask & PFM_SIZE) && is_field("Size", line)) {
- char *tmp = parse_simple("Size", line);
- pkg->size = strtoul(tmp, NULL, 0);
- free(tmp);
- } else if ((mask & PFM_SOURCE) && is_field("Source", line))
+ else if ((mask & PFM_SIZE) && is_field("Size", line))
+ pkg->size = parse_ulong(pkg, "Size", line);
+ else if ((mask & PFM_SOURCE) && is_field("Source", line))
pkg->source = parse_simple("Source", line);
else if ((mask & PFM_STATUS) && is_field("Status", line))
parse_status(pkg, line);
diff --git a/tests/Makefile b/tests/Makefile
index dd883c8..d69d041 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -95,6 +95,7 @@ REGRESSION_TESTS := core/01_install.py \
regress/issue11826.py \
regress/issue13574.py \
regress/issue13758.py \
+ regress/issue14178.py \
regress/issue14459.py \
misc/filehash.py \
misc/update_loses_autoinstalled_flag.py \
diff --git a/tests/regress/issue14178.py b/tests/regress/issue14178.py
new file mode 100755
index 0000000..02bd6b2
--- /dev/null
+++ b/tests/regress/issue14178.py
@@ -0,0 +1,77 @@
+#! /usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Reporter: Kay Hackmack
+#
+# Due to an error on our side our feed's Packages/Packages.gz was generated
+# like this (xyz is some package-specific data):
+#
+# Package: xyz
+# Version: 20.7.0.79-0+d79
+# Architecture: core2-64
+# Maintainer: Example User <us...@example.com>
+# Source: xyz
+# Section: Add-Ons
+# Priority: extra
+# Depends: xyz
+# Homepage: https://www.example.com/
+# DisplayVersion: 20.7
+# DisplayName: xyz
+# UserVisible: yes
+# Description: xyz
+# xyz
+# MD5Sum:
+# Size:
+# Filename: xyz.ipk
+#
+# After opkg on the target accessed/cached that feed it starts segfaulting
+# when running any command.
+# The key here seems to be that MD5Sum and/or Size: are empty. I'm not sure
+# which entry (MD5Sum or Size) is causing the crash or both at the moment.
+
+import os
+import opk, cfg, opkgcl
+
+BOGUS_SIZES = [
+ "", # empty string
+ "-1", # negative number
+ "-9223372036854775808", # LLONG_MIN - 1
+ "18446744073709551616", # ULLONG_MAX + 1
+ "foo", # no digits
+ "42fortytwo", # digits but also (non-hex) digits
+]
+
+for bogosity in BOGUS_SIZES:
+ opk.regress_init()
+
+ o = opk.OpkGroup()
+ o.add(Package="xyz", Version="1.0")
+ o.write_opk()
+
+ # write out a package list with bogus size
+ with open('Packages', 'w') as f:
+ for pkg in o.opk_list:
+ for k in pkg.control.keys():
+ f.write('{}: {}\n'.format(k, pkg.control[k]))
+ fname = pkg.control['Filename']
+ md5sum = opk.md5sum_file(fname)
+ f.write('Size: {}\n'.format(bogosity))
+ f.write('MD5Sum: {}\n'.format(md5sum))
+ f.write('\n')
+
+ if opkgcl.update() != 0:
+ fail("failed to update package list")
+
+ (status, output) = opkgcl.opkgcl('list')
+ if status != 0:
+ # Even with the ERROR printed this is still a success, in that
+ # it should not be returning code 139 (SIGSEGV)
+ opk.fail("opkg list returned nonzero exit code {}".format(status))
+ if "Failed to parse Size line for xyz" not in output:
+ opk.fail("expected to find error in output, but did not")
+
+ (status, output) = opkgcl.opkgcl('info xyz')
+ if status != 0:
+ opk.fail("opkg list returned nonzero exit code {}".format(status))
+ if "Failed to parse Size line for xyz" not in output:
+ opk.fail("expected to find error in output, but did not")
--
2.30.2

[Alex Stewart]

Looks good to me, and it passes testing [1].

I'll merge this on Monday, if there are no objections.

Thanks!


[1] https://gitlab.com/astewart.49c6/opkg/-/pipelines/611324040

> +# Homepage: https://urldefense.com/v3/__https://www.example.com/__;!!FbZ0ZwI3Qg!rQIEh4JM-KkE8EhjDYoIPcyuh3a_FnCKbMxd_rUY8l2hFZ6-vHs0sRM4r_iSUU58fYHXgj-I54BpJXMGFUl2vQ$

Alex Stewart
Software Engineer - NI Real-Time OS
NI (National Instruments)

alex.s...@ni.com

[Alex Stewart]

Merged; thanks.

https://git.yoctoproject.org/opkg/commit/?id=bdf303c54ccab0c0ff2dc1bcba00f5a0fd92aeff

On 8/10/22 16:50, Brenda Streiff wrote:

> +# Homepage: https://urldefense.com/v3/__https://www.example.com/__;!!FbZ0ZwI3Qg!rQIEh4JM-KkE8EhjDYoIPcyuh3a_FnCKbMxd_rUY8l2hFZ6-vHs0sRM4r_iSUU58fYHXgj-I54BpJXMGFUl2vQ$