diff mbox series

[v2] compare_versions: make is_oldstyle_version fail when number > 65535

Message ID 20211018233142.2385778-1-dominique.martinet@atmark-techno.com
State Accepted
Headers show
Series [v2] compare_versions: make is_oldstyle_version fail when number > 65535 | expand

Commit Message

Dominique Martinet Oct. 18, 2021, 11:31 p.m. UTC 
is_oldstyle_version would accept any string comprised of digits and dots.
Parse the actual version in is_oldstyle_version to detect large numbers and
fall back to semver if a large number was given, as semver can handle bigger
values.
Note that if this happens on the 4th level, this result in that number
actually being ignored as semver only handles up to major.minor.patch
levels.

Also print trace messages in case of parse failure (too many digits or number
too big) to facilitate debugging.

Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---
v1->v2:
- fix memory leak on early return
- use free_string_array()

 core/artifacts_versions.c | 74 +++++++++++++++++++++++----------------
 1 file changed, 43 insertions(+), 31 deletions(-)

Comments

Dominique Martinet Oct. 18, 2021, 11:42 p.m. UTC | #1 
Dominique Martinet wrote on Tue, Oct 19, 2021 at 08:31:42AM +0900:
> +		if (fld > 0xffff) {
> +			TRACE("Version %s had an element > 65535, falling back to semver",
> +			      version_string);

ah, sorry I forgot to make these DEBUG as you had said you did.

I do not see the patch in the coverity_scan branch (or otherwise) so
leaving that part up to you.


BTW I've also looked at your documentation update already in master, it
looks good to me -- thanks!
Stefano Babic Oct. 19, 2021, 10:33 a.m. UTC | #2 
Hi Dominique,

On 19.10.21 01:42, Dominique Martinet wrote:
> Dominique Martinet wrote on Tue, Oct 19, 2021 at 08:31:42AM +0900:
>> +		if (fld > 0xffff) {
>> +			TRACE("Version %s had an element > 65535, falling back to semver",
>> +			      version_string);
> 
> ah, sorry I forgot to make these DEBUG as you had said you did.

No problem, I fixed it myself.

> 
> I do not see the patch in the coverity_scan branch (or otherwise) so
> leaving that part up to you.

I use a gitlab server for these tests, it is also public and I set up a 
gitlab runner. Access is public for both sources (but well, it is just a 
mirror..) and pipelines:

https://source.denx.de/swupdate

I push coverity only on this server after travis changed conditions and 
it is less suitable for FOSS projects. Travis is not used anymore for CI.

Your patch is tested here:

https://source.denx.de/swupdate/swupdate/-/jobs/337560

Pipelines here:

https://source.denx.de/swupdate/swupdate/-/pipelines

The coverity_branch is just a "test" branch to run then coverity tool. 
It is not guaranteed that this branch is not rebased (it is a lot). If 
everything is fine, I merge then coverity into master and I publish it.

Status of coverity is here:

https://scan.coverity.com/projects/sbabic-swupdate?tab=overview

I tried to make everything open, so that everybody can access.

> 
> 
> BTW I've also looked at your documentation update already in master, it
> looks good to me -- thanks!

;-)

Best regards,
Stefano
diff mbox series

Patch

diff --git a/core/artifacts_versions.c b/core/artifacts_versions.c
index 433125862014..e93248d8b955 100644
--- a/core/artifacts_versions.c
+++ b/core/artifacts_versions.c
@@ -147,19 +147,6 @@  void get_sw_versions(swupdate_cfg_handle  __attribute__ ((__unused__))*handle,
 }
 #endif
 
-static const char ACCEPTED_CHARS[] = "0123456789.";
-
-static bool is_oldstyle_version(const char* version_string)
-{
-	while (*version_string)
-	{
-		if (strchr(ACCEPTED_CHARS, *version_string) == NULL)
-			return false;
-		++version_string;
-	}
-	return true;
-}
-
 /*
  * convert a version string into a number
  * version string is in the format:
@@ -170,30 +157,52 @@  static bool is_oldstyle_version(const char* version_string)
  * Also major.minor or major.minor.revision are allowed
  * The conversion generates a 64 bit value that can be compared
  */
-static __u64 version_to_number(const char *version_string)
+static bool version_to_number(const char *version_string, __u64 *version_number)
 {
 	char **versions = NULL;
 	char **ver;
 	unsigned int count = 0;
+	bool valid = false;
 	__u64 version = 0;
 
 	versions = string_split(version_string, '.');
-	for (ver = versions; *ver != NULL; ver++, count ++) {
-		if (count < 4) {
-			unsigned long int fld = strtoul(*ver, NULL, 10);
-			/* check for return of strtoul, mandatory */
-			if (fld != ULONG_MAX) {
-				fld &= 0xffff;
-				version = (version << 16) | fld;
-			}
+	for (ver = versions; *ver != NULL && count < 4; ver++, count++) {
+		unsigned long int fld = strtoul(*ver, NULL, 10);
+		/* check for return of strtoul, mandatory */
+		if (fld > 0xffff) {
+			TRACE("Version %s had an element > 65535, falling back to semver",
+			      version_string);
+			goto out;
 		}
-		free(*ver);
+		version = (version << 16) | fld;
 	}
-	if ((count < 4) && (count > 0))
+	if (count >= 4) {
+		TRACE("Version %s had more than 4 numbers, trailing numbers will be ignored",
+		      version_string);
+	} else if (count > 0) {
 		version <<= 16 * (4 - count);
-	free(versions);
+	}
+	*version_number = version;
+	valid = true;
 
-	return version;
+out:
+	free_string_array(versions);
+
+	return valid;
+}
+
+static const char ACCEPTED_CHARS[] = "0123456789.";
+
+static bool is_oldstyle_version(const char *version_string, __u64 *version_number)
+{
+	const char *ver = version_string;
+	while (*ver)
+	{
+		if (strchr(ACCEPTED_CHARS, *ver) == NULL)
+			return false;
+		++ver;
+	}
+	return version_to_number(version_string, version_number);;
 }
 
 /*
@@ -209,12 +218,15 @@  static __u64 version_to_number(const char *version_string)
  */
 int compare_versions(const char* left_version, const char* right_version)
 {
-	if (is_oldstyle_version(left_version) && is_oldstyle_version(right_version))
-	{
-		__u64 left_u64 = version_to_number(left_version);
-		__u64 right_u64 = version_to_number(right_version);
 
-		DEBUG("Comparing old-style versions '%s' <-> '%s'", left_version, right_version);
+	__u64 left_u64;
+	__u64 right_u64;
+
+	if (is_oldstyle_version(left_version, &left_u64)
+	    && is_oldstyle_version(right_version, &right_u64))
+	{
+		DEBUG("Comparing old-style versions '%s' <-> '%s'",
+		      left_version, right_version);
 		TRACE("Parsed: '%llu' <-> '%llu'", left_u64, right_u64);
 
 		if (left_u64 < right_u64)