Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

*) core: make ap_escape_quotes() work correctly on strings #298

Closed
wants to merge 2 commits into from
Closed

*) core: make ap_escape_quotes() work correctly on strings #298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e03541
*) core: make ap_escape_quotes() work correctly on strings
Mar 17, 2022
48a4227
Merge branch 'trunk' into ap_escape_quotes
Apr 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Expand Up @@ -357,6 +357,7 @@ Release
/test/sni
/test/httpdunit
/test/httpdunit.cases
/check/

# /test/unit/
/test/unit/*.tests
Expand Down
4 changes: 4 additions & 0 deletions changes-entries/core_ap_escape_quotes.txt
View file
Open in desktop
@@ -0,0 +1,4 @@
*) core: make ap_escape_quotes() work correctly on strings
with more than MAX_INT/2 characters, counting quotes double.
Credit to <generalbugs@zippenhop.com> for finding this.
[Stefan Eissing]
1 change: 1 addition & 0 deletions configure.in
View file
Open in desktop
Expand Up @@ -1108,4 +1108,5 @@ AC_MSG_NOTICE([summary of build options:
LDFLAGS: ${APACHE_CONF_SEL_LDFLAGS}
LIBS: ${APACHE_CONF_SEL_LIBS}
C preprocessor: ${APACHE_CONF_SEL_CPP}
Unit Tests: ${UNITTEST_LIBS:-not enabled}
])
13 changes: 8 additions & 5 deletions server/util.c
View file
Open in desktop
Expand Up @@ -2615,7 +2615,7 @@ AP_DECLARE(void) ap_content_type_tolower(char *str)
*/
AP_DECLARE(char *) ap_escape_quotes(apr_pool_t *p, const char *instring)
{
int newlen = 0;
apr_ssize_t extra = 0;
const char *inchr = instring;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Why apr_ssize_t and not apr_size_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra is used in a ptrdiff_t expression below, which is signed. apr_ssize_t is ptrdiff_t on all but the most ancients of platforms.

But is it correct to use? The question is, does it deal correctly with strings that are longer than ptrdiff_t can hold (lets call this PTRDIFF_MAX)?

  • an instring that is longer than PTRDIFF_MAX: this would mean trouble as (inchr - instring)in the palloc expression would overflow. Which is undefined behaviour in C. C defines that you can substract pointers into
    the same array to get a number. So, maybe we can ignore that one.
  • an instring that is almost as long as PTRDIFF_MAX with enough quote characters that the resulting length is larger than PTRDIFF_MAX. This is currently not checked.

I think we need a check that the expression (inchr - instring) + extra + 1 did not overflow, e.g. became < 0, to be totally safe. And for this, we need to calculdate in apr_ssize_t, I guess.

What do you think?

char *outchr, *outstring;

Expand All @@ -2624,21 +2624,24 @@ AP_DECLARE(char *) ap_escape_quotes(apr_pool_t *p, const char *instring)
* string up by an extra byte each time we find an unescaped ".
*/
while (*inchr != '\0') {
newlen++;
if (*inchr == '"') {
newlen++;
extra++;
}
/*
* If we find a slosh, and it's not the last byte in the string,
* it's escaping something - advance past both bytes.
*/
else if ((*inchr == '\\') && (inchr[1] != '\0')) {
inchr++;
newlen++;
}
inchr++;
}
outstring = apr_palloc(p, newlen + 1);

if (!extra) {
return apr_pstrdup(p, instring);
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe apr_pmemdup(), as we can easily compute the length, as done below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion.

outstring = apr_palloc(p, (inchr - instring) + extra + 1);
inchr = instring;
outchr = outstring;
/*
Expand Down
35 changes: 35 additions & 0 deletions test/unit/util.c
View file
Open in desktop
Expand Up @@ -79,6 +79,41 @@ HTTPD_START_LOOP_TEST(find_token_correctly_parses_token_list, ap_test_token_case
}
END_TEST


/*
* ap_escape_quotes()
*/

struct ap_escape_quotes_case {
const char *input;
const char *expected;
};

#define ESCQ "\\\""

const struct ap_escape_quotes_case ap_test_escape_quotes_cases[] = {
{ "one", "one" },
{ "o\"ne", "o"ESCQ"ne" },
{ "o"ESCQ"ne", "o"ESCQ"ne" },
{ "one\\", "one\\" },
{ "o\\ne", "o\\ne" },
{ "o\\"ESCQ"ne", "o\\\\"ESCQ"ne" }, /* 1st \ escapes 2nd, following '"' needs \ */
};

const size_t ap_test_escape_quotes_cases_len = sizeof(ap_test_escape_quotes_cases) /
sizeof(ap_test_escape_quotes_cases[0]);

HTTPD_START_LOOP_TEST(check_escape_quotes, ap_test_escape_quotes_cases_len)
{
const struct ap_escape_quotes_case *c = &ap_test_escape_quotes_cases[_i];
const char *result;

result = ap_escape_quotes(g_pool, c->input);
ck_assert_str_eq(result, c->expected);
}
END_TEST


/*
* Test Case Boilerplate
*/
Expand Down