[Asterisk-code-review] strings.h: Fix issues with escape string functions. (asterisk[certified/13.1])
Richard Mudgett
asteriskteam at digium.com
Thu Jul 16 12:27:26 CDT 2015
Richard Mudgett has uploaded a new change for review.
https://gerrit.asterisk.org/919
Change subject: strings.h: Fix issues with escape string functions.
......................................................................
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
---
M include/asterisk/strings.h
M main/utils.c
M tests/test_strings.c
3 files changed, 87 insertions(+), 28 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/19/919/1
diff --git a/include/asterisk/strings.h b/include/asterisk/strings.h
index cf3036a..e4594df 100644
--- a/include/asterisk/strings.h
+++ b/include/asterisk/strings.h
@@ -312,32 +312,33 @@
/*!
* \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 71d56e7..8305faa 100644
--- a/main/utils.c
+++ b/main/utils.c
@@ -1634,40 +1634,50 @@
'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 @@
* 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 @@
* 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 @@
{
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 fc8f38c..b115df0 100644
--- a/tests/test_strings.c
+++ b/tests/test_strings.c
@@ -457,7 +457,32 @@
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:
@@ -471,16 +496,27 @@
}
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;
}
--
To view, visit https://gerrit.asterisk.org/919
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id77fc704600ebcce81615c1200296f74de254104
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: certified/13.1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
More information about the asterisk-code-review
mailing list