gh-674: Optimize celix_version_parse to avoid unnecessary memory allo…

…cation.

When compiling a filter, it will try to parse arbitrary string as a version. The string length may exceed 64 bytes and thus will trigger unnecessary dynamic memory allocation.

libs/utils/gtest/src/ConvertUtilsTestSuite.cc

@@ -235,6 +235,7 @@ TEST_F(ConvertUtilsTestSuite, ConvertToVersionTest) {
     convertStatus = celix_utils_convertStringToVersion("A", nullptr, &result);
     EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, convertStatus);
     EXPECT_EQ(nullptr, result);
+    EXPECT_STREQ("Invalid version component(0)", celix_err_popLastError());
 
     //test for a string with a number
     convertStatus = celix_utils_convertStringToVersion("1.2.3.A", nullptr, &result);
@@ -270,11 +271,13 @@ TEST_F(ConvertUtilsTestSuite, ConvertToVersionTest) {
     EXPECT_NE(nullptr, result);
     checkVersion(result, 1, 2, 3, "B"); //default version
     celix_version_destroy(result);
+    EXPECT_STREQ("Invalid version component(0)", celix_err_popLastError());
 
     //test for a convert with a version value with trailing chars
     convertStatus = celix_utils_convertStringToVersion("2.1.1 and something else", nullptr, &result);
     EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, convertStatus);
     EXPECT_EQ(nullptr, result);
+    EXPECT_STREQ("Invalid trailing string:< and something else>", celix_err_popLastError());
 
     //test for a convert with a version value with trailing whitespaces
     convertStatus = celix_utils_convertStringToVersion("1.2.3 \t\n", nullptr, &result);
@@ -296,6 +299,7 @@ TEST_F(ConvertUtilsTestSuite, ConvertToVersionTest) {
     convertStatus = celix_utils_convertStringToVersion(longString.c_str(), nullptr, &result);
     EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, convertStatus);
     EXPECT_EQ(nullptr, result);
+    EXPECT_STREQ("Invalid version qualifier. Characters must be [A-Za-z0-9_-]", celix_err_popLastError());
 
     convertStatus = celix_utils_convertStringToVersion(nullptr, defaultVersion, &result);
     EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, convertStatus);

libs/utils/gtest/src/VersionTestSuite.cc

@@ -18,11 +18,16 @@
  */
 
 #include <gtest/gtest.h>
+#include <limits>
 
 #include "celix_version.h"
+#include "celix_err.h"
 
 class VersionTestSuite : public ::testing::Test {
 public:
+    ~VersionTestSuite() override {
+        celix_err_resetErrors();
+    }
     void expectVersion(const celix_version_t* version, int major, int minor, int micro, const char* qualifier = "") {
         if (version) {
             EXPECT_EQ(major, celix_version_getMajor(version));
@@ -257,9 +262,22 @@ TEST_F(VersionTestSuite, ParseTest) {
     expectVersion(result, 1, 0, 0);
     celix_version_destroy(result);
 
+    auto largeLong = std::to_string(std::numeric_limits<unsigned long long>::max());
+    parseStatus = celix_version_parse(largeLong.c_str(), &result);
+    EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, parseStatus);
+    EXPECT_EQ(nullptr, result);
+    EXPECT_STREQ("Invalid version component(0)", celix_err_popLastError());
+
+    auto largeInteger = std::to_string(std::numeric_limits<unsigned int>::max());
+    parseStatus = celix_version_parse(largeInteger.c_str(), &result);
+    EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, parseStatus);
+    EXPECT_EQ(nullptr, result);
+    EXPECT_STREQ("Invalid version component(0)", celix_err_popLastError());
+
     parseStatus = celix_version_parse("", &result);
     EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, parseStatus);
     EXPECT_EQ(nullptr, result);
+    EXPECT_STREQ("Invalid version component(0)", celix_err_popLastError());
 
     parseStatus = celix_version_parse(nullptr, &result);
     EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, parseStatus);
@@ -268,16 +286,20 @@ TEST_F(VersionTestSuite, ParseTest) {
     parseStatus = celix_version_parse("invalid", &result);
     EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, parseStatus);
     EXPECT_EQ(nullptr, result);
+    EXPECT_STREQ("Invalid version component(0)", celix_err_popLastError());
 
     parseStatus = celix_version_parse("-1", &result);
     EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, parseStatus);
     EXPECT_EQ(nullptr, result);
+    EXPECT_STREQ("Invalid version component(0)", celix_err_popLastError());
 
     parseStatus = celix_version_parse("1.-2", &result);
     EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, parseStatus);
     EXPECT_EQ(nullptr, result);
+    EXPECT_STREQ("Invalid version component(1)", celix_err_popLastError());
 
     parseStatus = celix_version_parse("1.2.-3", &result);
     EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, parseStatus);
     EXPECT_EQ(nullptr, result);
+    EXPECT_STREQ("Invalid version component(2)", celix_err_popLastError());
 }

libs/utils/src/celix_convert_utils.c

@@ -18,6 +18,7 @@
  */
 
 #include "celix_convert_utils.h"
+#include "celix_convert_utils_private.h"
 
 #include <assert.h>
 #include <ctype.h>
@@ -34,7 +35,7 @@
 #define ESCAPE_CHAR '\\'
 #define SEPARATOR_CHAR ','
 
-static bool celix_utils_isEndptrEndOfStringOrOnlyContainsWhitespaces(const char* endptr) {
+bool celix_utils_isEndptrEndOfStringOrOnlyContainsWhitespaces(const char* endptr) {
     bool result = false;
     if (endptr != NULL) {
         while (*endptr != '\0') {

libs/utils/src/celix_convert_utils_private.h

@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef CELIX_CELIX_CONVERT_UTILS_PRIVATE_H
+#define CELIX_CELIX_CONVERT_UTILS_PRIVATE_H
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+bool celix_utils_isEndptrEndOfStringOrOnlyContainsWhitespaces(const char* endptr);
+
+#ifdef __cplusplus
+}
+#endif
+#endif //CELIX_CELIX_CONVERT_UTILS_PRIVATE_H

libs/utils/src/filter.c

@@ -877,7 +877,7 @@ bool celix_filter_match(const celix_filter_t* filter, const celix_properties_t*
         return !childResult;
     }
 
-    // present, substring, equal, greater, greaterEqual, less, lessEqual, approx done with matchPropertyEntry
+    // substring, equal, greater, greaterEqual, less, lessEqual, approx done with matchPropertyEntry
     const celix_properties_entry_t* entry = celix_properties_getEntry(properties, filter->attribute);
     if (!entry) {
         return false;

libs/utils/src/version.c

@@ -19,11 +19,15 @@
 
 #include "celix_version.h"
 
+#include <ctype.h>
+#include <errno.h>
+#include <limits.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
-#include <celix_utils.h>
 
+#include "celix_utils.h"
+#include "celix_convert_utils_private.h"
 #include "celix_convert_utils.h"
 #include "celix_err.h"
 #include "celix_errno.h"
@@ -33,6 +37,7 @@ static const char* const CELIX_VERSION_EMPTY_QUALIFIER = "";
 
 celix_version_t* celix_version_create(int major, int minor, int micro, const char* qualifier) {
     if (major < 0 || minor < 0 || micro < 0) {
+        errno = EINVAL;
         celix_err_push("Invalid version number. Major, minor and micro must be >= 0");
         return NULL;
     }
@@ -56,6 +61,7 @@ celix_version_t* celix_version_create(int major, int minor, int micro, const cha
         if ((ch == '_') || (ch == '-')) {
             continue;
         }
+        errno = EINVAL;
         celix_err_push("Invalid version qualifier. Characters must be [A-Za-z0-9_-]");
         return NULL;
     }
@@ -106,52 +112,40 @@ celix_version_t* celix_version_createVersionFromString(const char *versionStr) {
     return version;
 }
 
-celix_status_t celix_version_parse(const char *versionStr, celix_version_t** version) {
+celix_status_t celix_version_parse(const char* versionStr, celix_version_t** version) {
     *version = NULL;
 
-    if (celix_utils_isStringNullOrEmpty(versionStr)) {
+    if (versionStr == NULL) {
         return CELIX_ILLEGAL_ARGUMENT;
     }
-
-    char buffer[64];
-    char* versionWrkStr = celix_utils_writeOrCreateString(buffer, sizeof(buffer), "%s", versionStr);
-    if (!versionWrkStr) {
-        celix_err_push("Failed to allocate memory for celix_version_createVersionFromString");
-        return CELIX_ENOMEM;
-    }
-
     int versionsParts[3] = {0, 0, 0};
-    char* qualifier = NULL;
-    char* savePtr = NULL;
-    char* token = strtok_r(versionWrkStr, ".", &savePtr);
     int count = 0;
-    while (token) {
-        bool convertedToLong = false;
-        long l = celix_utils_convertStringToLong(token, 0L, &convertedToLong);
-        if (!convertedToLong && count == 3) { // qualifier
-            qualifier = token;
-        } else if (convertedToLong && l < 0) {
-            //negative version part
-            celix_utils_freeStringIfNotEqual(buffer, versionWrkStr);
-            return CELIX_ILLEGAL_ARGUMENT;
-        } else if (convertedToLong && count < 3) {
-            versionsParts[count] = (int)l;
-        } else if (!convertedToLong) {
-            //unexpected token
-            celix_utils_freeStringIfNotEqual(buffer, versionWrkStr);
-            return CELIX_ILLEGAL_ARGUMENT;
+    const char* token = versionStr;
+
+    const char* qualifier = NULL;
+    while (token != NULL && count < 3) {
+        char* endPtr = NULL;
+        errno = 0;
+        long l = strtol(token, &endPtr, 10);
+        if (errno != 0 || token == endPtr || l < 0 || l >= INT_MAX) {
+            celix_err_pushf("Invalid version component(%d)", count);
+            return  CELIX_ILLEGAL_ARGUMENT;
+        }
+        versionsParts[count++] = (int)l;
+        if (*endPtr == '.') {
+            token = endPtr + 1;
+        } else if (celix_utils_isEndptrEndOfStringOrOnlyContainsWhitespaces(endPtr)){
+            token = NULL;
         } else {
-            //to many version parts
-            celix_utils_freeStringIfNotEqual(buffer, versionWrkStr);
+            celix_err_pushf("Invalid trailing string:<%s>", endPtr);
             return CELIX_ILLEGAL_ARGUMENT;
         }
-        count += 1;
-        token = strtok_r(NULL, ".", &savePtr);
     }
-
+    if (token != NULL) {
+        qualifier = token;
+    }
     *version = celix_version_create(versionsParts[0], versionsParts[1], versionsParts[2], qualifier);
-    celix_utils_freeStringIfNotEqual(buffer, versionWrkStr);
-    return *version ? CELIX_SUCCESS : CELIX_ENOMEM;
+    return *version ? CELIX_SUCCESS : (errno == EINVAL ? CELIX_ILLEGAL_ARGUMENT : CELIX_ENOMEM);
 }
 
 celix_version_t* celix_version_createEmptyVersion() {