strings.h: Fix issues with escape string functions.
authorRichard Mudgett <rmudgett@digium.com>
Wed, 15 Jul 2015 20:40:32 +0000 (15:40 -0500)
committerRichard Mudgett <rmudgett@digium.com>
Thu, 16 Jul 2015 17:27:01 +0000 (12:27 -0500)
Fixes for issues with the ASTERISK-24934 patch.

* Fixed ast_escape_alloc() and ast_escape_c_alloc() if the s parameter is
an empty string.  If it were an empty string the functions returned NULL
as if there were a memory allocation failure.  This failure caused the AMI
VarSet event to not get posted if the new value was an empty string.

* Fixed dest buffer overwrite potential in ast_escape() and
ast_escape_c().  If the dest buffer size is smaller than the space needed
by the escaped s parameter string then the dest buffer would be written
beyond the end by the nul string terminator.  The num parameter was really
the dest buffer size parameter so I renamed it to size.

* Made nul terminate the dest buffer if the source string parameter s was
an empty string in ast_escape() and ast_escape_c().

* Updated ast_escape() and ast_escape_c() doxygen function description
comments to reflect reality.

* Added some more unit test cases to /main/strings/escape to cover the
empty source string issues.

ASTERISK-25255 #close
Reported by: Richard Mudgett

Change-Id: Id77fc704600ebcce81615c1200296f74de254104

include/asterisk/strings.h
main/utils.c
tests/test_strings.c

index d361293..af5ae6c 100644 (file)
@@ -312,32 +312,33 @@ char *ast_unescape_c(char *s);
 /*!
  * \brief Escape the 'to_escape' characters in the given string.
  *
- * \note The given output buffer has to have enough memory allocated to store the
- *       original string plus any escaped values.
+ * \note The given output buffer will contain a truncated escaped
+ * version of the source string if the given buffer is not large
+ * enough.
  *
  * \param dest the escaped string
  * \param s the source string to escape
- * \param num number of characters to be copied from the source
+ * \param size The size of the destination buffer
  * \param to_escape an array of characters to escape
  *
  * \return Pointer to the destination.
  */
-char* ast_escape(char *dest, const char *s, size_t num, const char *to_escape);
+char *ast_escape(char *dest, const char *s, size_t size, const char *to_escape);
 
 /*!
  * \brief Escape standard 'C' sequences in the given string.
  *
- * \note The given output buffer has to have enough memory allocated to store the
- *       original string plus any escaped values.
+ * \note The given output buffer will contain a truncated escaped
+ * version of the source string if the given buffer is not large
+ * enough.
  *
  * \param dest the escaped string
  * \param s the source string to escape
- * \param num number of characters to be copied from the source
- * \param to_escape an array of characters to escape
+ * \param size The size of the destination buffer
  *
  * \return Pointer to the escaped string.
  */
-char* ast_escape_c(char *dest, const char *s, size_t num);
+char *ast_escape_c(char *dest, const char *s, size_t size);
 
 /*!
  * \brief Escape the 'to_escape' characters in the given string.
index 3bf2c51..4bcfa33 100644 (file)
@@ -1634,40 +1634,50 @@ static char escape_sequences_map[] = {
        'a', 'b', 'f', 'n', 'r', 't', 'v', '\\', '\'', '"', '?', '\0'
 };
 
-char* ast_escape(char *dest, const char *s, size_t num, const char *to_escape)
+char *ast_escape(char *dest, const char *s, size_t size, const char *to_escape)
 {
        char *p;
+       char *c;
 
-       if (!dest || ast_strlen_zero(s)) {
+       if (!dest || !size) {
+               return dest;
+       }
+       if (ast_strlen_zero(s)) {
+               *dest = '\0';
                return dest;
        }
 
        if (ast_strlen_zero(to_escape)) {
-               ast_copy_string(dest, s, num);
+               ast_copy_string(dest, s, size);
                return dest;
        }
 
-       for (p = dest; *s && num--; ++s, ++p) {
+       for (p = dest; *s && --size; ++s, ++p) {
                /* If in the list of characters to escape then escape it */
                if (strchr(to_escape, *s)) {
+                       if (!--size) {
+                               /* Not enough room left for the escape sequence. */
+                               break;
+                       }
+
                        /*
                         * See if the character to escape is part of the standard escape
                         * sequences. If so we'll have to use its mapped counterpart
                         * otherwise just use the current character.
                         */
-                       char *c = strchr(escape_sequences, *s);
+                       c = strchr(escape_sequences, *s);
                        *p++ = '\\';
                        *p = c ? escape_sequences_map[c - escape_sequences] : *s;
                } else {
                        *p = *s;
                }
        }
-
        *p = '\0';
+
        return dest;
 }
 
-char* ast_escape_c(char *dest, const char *s, size_t num)
+char *ast_escape_c(char *dest, const char *s, size_t size)
 {
        /*
         * Note - This is an optimized version of ast_escape. When looking only
@@ -1675,32 +1685,42 @@ char* ast_escape_c(char *dest, const char *s, size_t num)
         * be left out thus making it slightly more efficient.
         */
        char *p;
+       char *c;
 
-       if (!dest || ast_strlen_zero(s)) {
+       if (!dest || !size) {
+               return dest;
+       }
+       if (ast_strlen_zero(s)) {
+               *dest = '\0';
                return dest;
        }
 
-       for (p = dest; *s && num--; ++s, ++p) {
+       for (p = dest; *s && --size; ++s, ++p) {
                /*
                 * See if the character to escape is part of the standard escape
                 * sequences. If so use its mapped counterpart.
                 */
-               char *c = strchr(escape_sequences, *s);
+               c = strchr(escape_sequences, *s);
                if (c) {
+                       if (!--size) {
+                               /* Not enough room left for the escape sequence. */
+                               break;
+                       }
+
                        *p++ = '\\';
                        *p = escape_sequences_map[c - escape_sequences];
                } else {
                        *p = *s;
                }
        }
-
        *p = '\0';
+
        return dest;
 }
 
 static char *escape_alloc(const char *s, size_t *size)
 {
-       if (!s || !(*size = strlen(s))) {
+       if (!s) {
                return NULL;
        }
 
@@ -1708,14 +1728,15 @@ static char *escape_alloc(const char *s, size_t *size)
         * The result string needs to be twice the size of the given
         * string just in case every character in it needs to be escaped.
         */
-       *size = *size * 2 + 1;
-       return ast_calloc(sizeof(char), *size);
+       *size = strlen(s) * 2 + 1;
+       return ast_malloc(*size);
 }
 
 char *ast_escape_alloc(const char *s, const char *to_escape)
 {
        size_t size = 0;
        char *dest = escape_alloc(s, &size);
+
        return ast_escape(dest, s, size, to_escape);
 }
 
@@ -1723,6 +1744,7 @@ char *ast_escape_c_alloc(const char *s)
 {
        size_t size = 0;
        char *dest = escape_alloc(s, &size);
+
        return ast_escape_c(dest, s, size);
 }
 
index d1ff2f0..a39ac63 100644 (file)
@@ -460,7 +460,32 @@ AST_TEST_DEFINE(escape_test)
        char buf[128];
 
 #define TEST_ESCAPE(s, to_escape, expected) \
-       !strcmp(ast_escape(buf, s, sizeof(buf) / sizeof(char), to_escape), expected)
+       !strcmp(ast_escape(buf, s, ARRAY_LEN(buf), to_escape), expected)
+
+#define TEST_ESCAPE_C(s, expected) \
+       !strcmp(ast_escape_c(buf, s, ARRAY_LEN(buf)), expected)
+
+#define TEST_ESCAPE_ALLOC(s, to_escape, expected)              \
+       ({                                                                                                      \
+               int res = 0;                                                                    \
+               char *a_buf = ast_escape_alloc(s, to_escape);   \
+               if (a_buf) {                                                                    \
+                       res = !strcmp(a_buf, expected);                         \
+                       ast_free(a_buf);                                                        \
+               }                                                                                               \
+               res;                                                                                    \
+       })
+
+#define TEST_ESCAPE_C_ALLOC(s, expected)                               \
+       ({                                                                                                      \
+               int res = 0;                                                                    \
+               char *a_buf = ast_escape_c_alloc(s);                    \
+               if (a_buf) {                                                                    \
+                       res = !strcmp(a_buf, expected);                         \
+                       ast_free(a_buf);                                                        \
+               }                                                                                               \
+               res;                                                                                    \
+       })
 
        switch (cmd) {
        case TEST_INIT:
@@ -474,16 +499,27 @@ AST_TEST_DEFINE(escape_test)
        }
 
        ast_test_validate(test, TEST_ESCAPE("null escape", NULL, "null escape"));
+       ast_test_validate(test, TEST_ESCAPE("empty escape", "", "empty escape"));
+       ast_test_validate(test, TEST_ESCAPE("", "Z", ""));
        ast_test_validate(test, TEST_ESCAPE("no matching escape", "Z", "no matching escape"));
        ast_test_validate(test, TEST_ESCAPE("escape Z", "Z", "escape \\Z"));
        ast_test_validate(test, TEST_ESCAPE("Z", "Z", "\\Z"));
-       ast_test_validate(test, TEST_ESCAPE(";;", ";;", "\\;\\;"));
+       ast_test_validate(test, TEST_ESCAPE(";;", ";", "\\;\\;"));
        ast_test_validate(test, TEST_ESCAPE("escape \n", "\n", "escape \\n"));
        ast_test_validate(test, TEST_ESCAPE("escape \n again \n", "\n", "escape \\n again \\n"));
 
-       ast_test_validate(test, !strcmp(ast_escape_c(buf, "escape \a\b\f\n\r\t\v\\\'\"\?",
-                                                    sizeof(buf) / sizeof(char)),
-                                       "escape \\a\\b\\f\\n\\r\\t\\v\\\\\\\'\\\"\\?"));
+       ast_test_validate(test, TEST_ESCAPE_C("", ""));
+       ast_test_validate(test, TEST_ESCAPE_C("escape \a\b\f\n\r\t\v\\\'\"\?",
+               "escape \\a\\b\\f\\n\\r\\t\\v\\\\\\\'\\\"\\?"));
+
+       ast_test_validate(test, TEST_ESCAPE_ALLOC("", "Z", ""));
+       ast_test_validate(test, TEST_ESCAPE_ALLOC("Z", "Z", "\\Z"));
+       ast_test_validate(test, TEST_ESCAPE_ALLOC("a", "Z", "a"));
+
+       ast_test_validate(test, TEST_ESCAPE_C_ALLOC("", ""));
+       ast_test_validate(test, TEST_ESCAPE_C_ALLOC("\n", "\\n"));
+       ast_test_validate(test, TEST_ESCAPE_C_ALLOC("a", "a"));
+
        return AST_TEST_PASS;
 }