From e31cb6b2484bbf5726c59b263f13b995e60d537d Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Wed, 15 Jul 2015 15:40:32 -0500 Subject: strings.h: Fix issues with escape string functions. 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 | 19 +++++++++--------- main/utils.c | 50 +++++++++++++++++++++++++++++++++------------- tests/test_strings.c | 46 +++++++++++++++++++++++++++++++++++++----- 3 files changed, 87 insertions(+), 28 deletions(-) diff --git a/include/asterisk/strings.h b/include/asterisk/strings.h index d361293d0..af5ae6c55 100644 --- a/include/asterisk/strings.h +++ b/include/asterisk/strings.h @@ -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. diff --git a/main/utils.c b/main/utils.c index 67c391ce2..f9a5be26c 100644 --- a/main/utils.c +++ b/main/utils.c @@ -1639,40 +1639,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 @@ -1680,32 +1690,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; } @@ -1713,14 +1733,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); } @@ -1728,6 +1749,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); } diff --git a/tests/test_strings.c b/tests/test_strings.c index ab65037ee..5e3446d99 100644 --- a/tests/test_strings.c +++ b/tests/test_strings.c @@ -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; } -- cgit v1.2.3