colin/NetworkManager
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

ifcfg-rh: use NMStrBuf in svUnescape()

Browse Source 
This is a popular, low-level function. Let's use NMStrBuf.

Also, Coverity wrongly things that there is a leak here. This change
should also avoid that:

    Error: RESOURCE_LEAK (CWE-772):
    NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/shvar.c:411: alloc_arg: "_gstr_init" allocates memory that is stored into "str".
    NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/shvar.c:423: noescape: Resource "str" is not freed or pointed-to in "g_string_append_len".
    NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/shvar.c:619: leaked_storage: Variable "str" going out of scope leaks the storage it points to.
    #  617|           nm_assert(!str);
    #  618|           *to_free = NULL;
    #  619|->         return "";
    #  620|       }
    #  621|

Profile:

We run test-ifcfg-rh which calls svUnescape() under realistic circumstances.
However, the test does too many other things that svUnescape() would be
measurable. So use the following patch, to run the tested code more frequently:

    diff --git a/src/core/settings/plugins/ifcfg-rh/shvar.c b/src/core/settings/plugins/ifcfg-rh/shvar.c
    index c6099dd1731c..18a907113ea9 100644
    --- a/src/core/settings/plugins/ifcfg-rh/shvar.c
    +++ b/src/core/settings/plugins/ifcfg-rh/shvar.c
    @@ -645,6 +645,24 @@ out_error:
         return NULL;
     }

    +#define svUnescape(value, to_free)                   \
    +    ({                                               \
    +        const char *_value = (value);                \
    +        const char *_result;                         \
    +        int         _i;                              \
    +                                                     \
    +        for (_i = 0; TRUE; _i++) {                   \
    +            gs_free char *_to_free;                  \
    +                                                     \
    +            _result = svUnescape(_value, &_to_free); \
    +            if (_i < 1000)                           \
    +                continue;                            \
    +            *(to_free) = g_steal_pointer(&_to_free); \
    +            break;                                   \
    +        }                                            \
    +        _result;                                     \
    +    })
    +
     /*****************************************************************************/

     shvarFile *

Build:

    CFLAGS='-O2' ./autogen.sh --with-more-asserts=0
    make -j 10 src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh && \
        src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh &&
        perf stat -r 50 -B src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh

Before:

 Performance counter stats for 'src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh' (20 runs):

            590.56 msec task-clock:u              #    0.972 CPUs utilized            ( +-  0.48% )
                 0      context-switches:u        #    0.000 K/sec
                 0      cpu-migrations:u          #    0.000 K/sec
             1,091      page-faults:u             #    0.002 M/sec                    ( +-  0.12% )
     2,022,618,453      cycles:u                  #    3.425 GHz                      ( +-  0.33% )
     4,165,011,633      instructions:u            #    2.06  insn per cycle           ( +-  0.01% )
     1,168,673,648      branches:u                # 1978.910 M/sec                    ( +-  0.01% )
         8,279,364      branch-misses:u           #    0.71% of all branches          ( +-  0.14% )

           0.60739 +- 0.00292 seconds time elapsed  ( +-  0.48% )

After:

 Performance counter stats for 'src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh' (50 runs):

            580.19 msec task-clock:u              #    0.972 CPUs utilized            ( +-  0.33% )
                 0      context-switches:u        #    0.000 K/sec
                 0      cpu-migrations:u          #    0.000 K/sec
             1,092      page-faults:u             #    0.002 M/sec                    ( +-  0.08% )
     1,956,368,933      cycles:u                  #    3.372 GHz                      ( +-  0.22% )
     4,106,984,148      instructions:u            #    2.10  insn per cycle           ( +-  0.01% )
     1,087,931,864      branches:u                # 1875.143 M/sec                    ( +-  0.01% )
         7,731,041      branch-misses:u           #    0.71% of all branches          ( +-  0.15% )

           0.59680 +- 0.00193 seconds time elapsed  ( +-  0.32% )

The run time varies greatly. But it can be seen that the new code is consistently
faster.
This commit is contained in:
Thomas Haller
2021-05-06 17:18:03 +02:00
parent 1556732ef0
commit dd3aa1224a
1 changed files with 35 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

71
src/core/settings/plugins/ifcfg-rh/shvar.c
View File

@@ -14,6 +14,7 @@
#include <sys/stat.h> #include <sys/stat.h>
#include <unistd.h> #include <unistd.h>
#include "libnm-glib-aux/nm-str-buf.h"
#include "libnm-core-intern/nm-core-internal.h" #include "libnm-core-intern/nm-core-internal.h"
#include "nm-core-utils.h" #include "nm-core-utils.h"
#include "libnm-glib-aux/nm-enum-utils.h" #include "libnm-glib-aux/nm-enum-utils.h"
@@ -318,14 +319,14 @@ _ch_hex_get(char ch)
} }
static void static void
_gstr_init(GString **str, const char *value, gsize i) _strbuf_init(NMStrBuf *str, const char *value, gsize i)
{ {
nm_assert(str); nm_assert(str);
nm_assert(value); nm_assert(value);
if (!(*str)) { if (str->allocated == 0) {
/* if @str is not yet initialized, it allocates /* if @str is not yet initialized, it initializes
* a new GString and copies @i characters from * a new NMStrBuf and copies @i characters from
* @value over. * @value over.
* *
* Unescaping usually does not extend the length of a string, * Unescaping usually does not extend the length of a string,
@@ -335,20 +336,20 @@ _gstr_init(GString **str, const char *value, gsize i)
* (FACTOR*strlen(value) + CONST), which is non trivial to get * (FACTOR*strlen(value) + CONST), which is non trivial to get
* right in all cases. Also, we would have to provision for the * right in all cases. Also, we would have to provision for the
* very unlikely extreme case. * very unlikely extreme case.
* Instead, use a GString buffer which can grow as needed. But for an * Instead, use a NMStrBuf buffer which can grow as needed. But for an
* initial guess, strlen(value) is a good start */ * initial guess, strlen(value) is a good start */
*str = g_string_new_len(NULL, strlen(value) + 3); nm_str_buf_maybe_expand(str, strlen(value) + 3u, FALSE);
if (i) nm_str_buf_append_len(str, value, i);
g_string_append_len(*str, value, i);
} }
} }
const char * const char *
svUnescape(const char *value, char **to_free) svUnescape(const char *value, char **to_free)
{ {
gsize i, j; NMStrBuf str = NM_STR_BUF_INIT(0, FALSE);
GString *str = NULL;
int looks_like_old_svescaped = -1; int looks_like_old_svescaped = -1;
gsize i;
gsize j;
/* we handle bash syntax here (note that ifup has #!/bin/bash. /* we handle bash syntax here (note that ifup has #!/bin/bash.
* Thus, see https://www.gnu.org/software/bash/manual/html_node/Quoting.html#Quoting */ * Thus, see https://www.gnu.org/software/bash/manual/html_node/Quoting.html#Quoting */
@@ -395,20 +396,20 @@ svUnescape(const char *value, char **to_free)
if (value[i] == '\\') { if (value[i] == '\\') {
/* backslash escape */ /* backslash escape */
_gstr_init(&str, value, i); _strbuf_init(&str, value, i);
i++; i++;
if (G_UNLIKELY(value[i] == '\0')) { if (G_UNLIKELY(value[i] == '\0')) {
/* we don't support line continuation */ /* we don't support line continuation */
goto out_error; goto out_error;
} }
g_string_append_c(str, value[i]); nm_str_buf_append_c(&str, value[i]);
i++; i++;
goto loop1_next; goto loop1_next;
} }
if (value[i] == '\'') { if (value[i] == '\'') {
/* single quotes */ /* single quotes */
_gstr_init(&str, value, i); _strbuf_init(&str, value, i);
i++; i++;
j = i; j = i;
while (TRUE) { while (TRUE) {
@@ -420,14 +421,14 @@ svUnescape(const char *value, char **to_free)
break; break;
j++; j++;
} }
g_string_append_len(str, &value[i], j - i); nm_str_buf_append_len(&str, &value[i], j - i);
i = j + 1; i = j + 1;
goto loop1_next; goto loop1_next;
} }
if (value[i] == '"') { if (value[i] == '"') {
/* double quotes */ /* double quotes */
_gstr_init(&str, value, i); _strbuf_init(&str, value, i);
i++; i++;
while (TRUE) { while (TRUE) {
if (value[i] == '"') { if (value[i] == '"') {
@@ -466,11 +467,11 @@ svUnescape(const char *value, char **to_free)
if (looks_like_old_svescaped < 0) if (looks_like_old_svescaped < 0)
looks_like_old_svescaped = _looks_like_old_svescaped(value); looks_like_old_svescaped = _looks_like_old_svescaped(value);
if (!looks_like_old_svescaped) if (!looks_like_old_svescaped)
g_string_append_c(str, '\\'); nm_str_buf_append_c(&str, '\\');
} else } else
g_string_append_c(str, '\\'); nm_str_buf_append_c(&str, '\\');
} }
g_string_append_c(str, value[i]); nm_str_buf_append_c(&str, value[i]);
i++; i++;
} }
goto loop1_next; goto loop1_next;
@@ -478,7 +479,7 @@ svUnescape(const char *value, char **to_free)
if (value[i] == '$' && value[i + 1] == '\'') { if (value[i] == '$' && value[i + 1] == '\'') {
/* ANSI-C Quoting */ /* ANSI-C Quoting */
_gstr_init(&str, value, i); _strbuf_init(&str, value, i);
i += 2; i += 2;
while (TRUE) { while (TRUE) {
char ch; char ch;
@@ -552,7 +553,7 @@ svUnescape(const char *value, char **to_free)
} }
} }
/* like bash, we cut too large numbers off. E.g. A=$'\772' becomes 0xfa */ /* like bash, we cut too large numbers off. E.g. A=$'\772' becomes 0xfa */
g_string_append_c(str, (guint8) v); nm_str_buf_append_c(&str, (guint8) v);
} else if (NM_IN_SET(value[i], 'x', 'u', 'U')) { } else if (NM_IN_SET(value[i], 'x', 'u', 'U')) {
const char escape_type = value[i]; const char escape_type = value[i];
int max_digits = escape_type == 'x' ? 2 : escape_type == 'u' ? 4 : 8; int max_digits = escape_type == 'x' ? 2 : escape_type == 'u' ? 4 : 8;
@@ -561,8 +562,7 @@ svUnescape(const char *value, char **to_free)
i++; i++;
if (!_ch_hex_is(value[i])) { if (!_ch_hex_is(value[i])) {
/* missing hex value after "\x" escape. This is treated like no escaping. */ /* missing hex value after "\x" escape. This is treated like no escaping. */
g_string_append_c(str, '\\'); nm_str_buf_append_c(&str, '\\', escape_type);
g_string_append_c(str, escape_type);
} else { } else {
v = _ch_hex_get(value[i]); v = _ch_hex_get(value[i]);
i++; i++;
@@ -574,22 +574,21 @@ svUnescape(const char *value, char **to_free)
i++; i++;
} }
if (escape_type == 'x') if (escape_type == 'x')
g_string_append_c(str, v); nm_str_buf_append_c(&str, v);
else { else {
/* we treat the unicode escapes as utf-8 encoded values. */ /* we treat the unicode escapes as utf-8 encoded values. */
g_string_append_unichar(str, v); nm_str_buf_append_unichar(&str, v);
} }
} }
} else { } else {
g_string_append_c(str, '\\'); nm_str_buf_append_c(&str, '\\', value[i]);
g_string_append_c(str, value[i]);
i++; i++;
} }
goto loop_ansic_next; goto loop_ansic_next;
} }
} else } else
ch = value[i]; ch = value[i];
g_string_append_c(str, ch); nm_str_buf_append_c(&str, ch);
i++; i++;
loop_ansic_next:; loop_ansic_next:;
} }
@@ -603,8 +602,8 @@ loop_ansic_next:;
} }
/* an unquoted, regular character. Just consume it directly. */ /* an unquoted, regular character. Just consume it directly. */
if (str) if (str.allocated > 0)
g_string_append_c(str, value[i]); nm_str_buf_append_c(&str, value[i]);
i++; i++;
loop1_next:; loop1_next:;
@@ -614,18 +613,19 @@ loop1_next:;
out_value: out_value:
if (i == 0) { if (i == 0) {
nm_assert(!str); nm_assert(str.allocated == 0);
nm_assert(!str._priv_str);
*to_free = NULL; *to_free = NULL;
return ""; return "";
} }
if (str) { if (str.allocated > 0) {
if (str->len == 0 || str->str[0] == '\0') { if (str.len == 0 || nm_str_buf_get_str_unsafe(&str)[0] == '\0') {
g_string_free(str, TRUE); nm_str_buf_destroy(&str);
*to_free = NULL; *to_free = NULL;
return ""; return "";
} else { } else {
*to_free = g_string_free(str, FALSE); *to_free = nm_str_buf_finalize(&str, NULL);
return *to_free; return *to_free;
} }
} }
@@ -639,8 +639,7 @@ out_value:
return value; return value;
out_error: out_error:
if (str) nm_str_buf_destroy(&str);
g_string_free(str, TRUE);
*to_free = NULL; *to_free = NULL;
return NULL; return NULL;
} }