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

fixups for sticky cookie calculation in case of segmented data #1088

Merged
merged 3 commits into from
Oct 30, 2018
Merged

fixups for sticky cookie calculation in case of segmented data #1088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa50fde
make tfw_str_collect_cmp() reset its output variable before use
i-rinat Oct 30, 2018
b1b6f7d
handle multiple chunks of sticky cookie string
i-rinat Oct 30, 2018
ed82007
generate a new sticky cookie if client sent a truncated cookie
i-rinat Oct 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions tempesta_fw/http_sess.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,14 +517,20 @@ tfw_http_redir_mark_get(TfwHttpReq *req, TfwStr *out_val)
#define HEX_STR_TO_BIN_GET(obj, f) \
({ \
int count = 0; \
if (!tr && c < end) \
if (c >= end) \
goto end_##f; \
if (!tr) \
tr = c->ptr; \
for ( ; c < end; ++c) { \
for (;;) { \
for ( ; tr < (unsigned char *)c->ptr + c->len; ++tr) { \
if (count++ == sizeof((obj)->f) * 2) \
goto end_##f; \
(obj)->f = ((obj)->f << 4) + hex_to_bin(*tr); \
} \
++c; \
if (c >= end) \
break; \
tr = c->ptr; \
} \
end_##f: \
; \
Expand All @@ -533,8 +539,11 @@ end_##f: \
#define HEX_STR_TO_BIN_HMAC(hmac, ts, addr) \
({ \
unsigned char b; \
int i, hi, r = TFW_HTTP_SESS_SUCCESS; \
for (i = 0, hi = 1; c < end; ++c) { \
int i = 0, hi = 1, r = TFW_HTTP_SESS_SUCCESS; \
\
if (c >= end) \
goto end; \
for (;;) { \
for ( ; tr < (unsigned char *)c->ptr + c->len; ++tr) { \
b = hi ? hex_asc_hi((hmac)[i]) \
: hex_asc_lo((hmac)[i]); \
Expand All @@ -552,6 +561,10 @@ end_##f: \
hi = !hi; \
i += hi; \
} \
++c; \
if (c >= end) \
break; \
tr = c->ptr; \
} \
BUG_ON(i != STICKY_KEY_MAXLEN); \
end: \
Expand Down Expand Up @@ -696,6 +709,7 @@ tfw_http_sticky_verify(TfwHttpReq *req, TfwStr *value, StickyVal *sv)
if (value->len != sizeof(StickyVal) * 2) {
sess_warn("bad sticky cookie length", addr, ": %lu(%lu)\n",
value->len, sizeof(StickyVal) * 2);
tfw_http_sticky_calc(req, sv);
return TFW_HTTP_SESS_VIOLATE;
}

Expand Down
12 changes: 10 additions & 2 deletions tempesta_fw/str.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,11 @@ void tfw_str_collect_cmp(TfwStr *chunk, TfwStr *end, TfwStr *out,

BUG_ON(!TFW_STR_PLAIN(chunk));

if (unlikely(chunk == end)) {
bzero_fast(out, sizeof(*out));
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This function was intended to collect cookie value in any case (or BUG_ON must be triggered), so it didn't return value. But now it can return with no value found and caller will not know anything about this. Perhaps it is worth to change it to return true/false or 0/-ENOENT; also the similar checks before tfw_str_collect_cmp calls - looks redundant now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't shake the feeling that it's wrong to add any return value to this function. It has a verb in the name, so by convention should return 0 if there are no errors, and other values otherwise. But this function cannot fail. It's not an error to return empty string if an empty slice was provided. If we add a return value, somebody reading code at the call site will get an impression of error handling, which it is not.

So I think it is good to leave testing for chunk == end before calling this function. It's a duplication, but I don't think checking should be removed from this function, as it may result in accidental reading outside of allocated memory.

/* If this is last chunk, just return it in this case. */
next = chunk + 1;
if (likely(next == end || (stop && *(char *)next->ptr == *stop))) {
Expand All @@ -149,8 +154,11 @@ void tfw_str_collect_cmp(TfwStr *chunk, TfwStr *end, TfwStr *out,

/* Add chunks to out-string. */
out->ptr = chunk;
TFW_STR_CHUNKN_ADD(out, 1);
out->len = chunk->len;
out->flags = 0;
out->len = 0;
out->eolen = 0;
/* __TFW_STR_CHUNKN_SET(out, 0); is done by out->flags = 0 */

for (; chunk != end; ++chunk) {
if (stop && *(char *)chunk->ptr == *stop)
break;
Expand Down
79 changes: 79 additions & 0 deletions tempesta_fw/t/unit/test_tfw_str.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1100,6 +1100,84 @@ TEST(tfw_str_crc32, plain_compound)
EXPECT_EQ(crc_pln, crc_cmpnd);
}

TEST(tfw_str_collect_cmp, collect_chunks)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not the issue of this PR actually, but can you add one more test for the http parser? Cookie calculations add several requirements for the http parser:

  • cookie name starts at a new chunk, and this chunk is marked with TFW_STR_NAME flag
  • cookie value starts at a new chunk, and this chunk is marked with TFW_STR_VALUE flag
  • cookie delimeter character(;) starts at a new chunk

This requirements aren't checked anywhere, but http_sess.c code highly relies on it.

Copy link
Contributor Author

@i-rinat i-rinat Oct 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ack. I'll add it.

{
TfwStr in = {
.ptr = (TfwStr []){
TFW_STR_FROM("abcd"),
TFW_STR_FROM("efghi"),
TFW_STR_FROM("jklmnopq"),
TFW_STR_FROM("rst"),
TFW_STR_FROM("uvwxyz")
},
.len = sizeof("abcdefghijklmnopqrstuvwxyz") - 1,
.flags = 5 << TFW_STR_CN_SHIFT
};
TfwStr *chunks = in.ptr;
TfwStr out = { .ptr = (void *)123, .skb = (void *)456, .len = 789,
.eolen = 10, .flags = 1112 };

tfw_str_collect_cmp(chunks, chunks + 5, &out, NULL);
EXPECT_TRUE(tfw_str_eq_cstr(&out, "abcdefghijklmnopqrstuvwxyz", 26, 0));
EXPECT_EQ(TFW_STR_CHUNKN(&out), 5);
EXPECT_EQ(out.len, 26);
/*
* tfw_str_collect_cmp() is expected to clear previous values from all
* other fields of the output TfwStr.
*/
EXPECT_EQ(out.eolen, 0);
EXPECT_EQ(out.flags & TFW_STR_FMASK, 0);

/*
* Try to start at other chunks too.
* Deliberately not reinitializing 'out' here to check that its previous
* contents is discarded.
*/
tfw_str_collect_cmp(chunks + 1, chunks + 5, &out, NULL);
EXPECT_TRUE(tfw_str_eq_cstr(&out, "efghijklmnopqrstuvwxyz", 22, 0));
EXPECT_EQ(TFW_STR_CHUNKN(&out), 4);

tfw_str_collect_cmp(chunks + 2, chunks + 5, &out, NULL);
EXPECT_TRUE(tfw_str_eq_cstr(&out, "jklmnopqrstuvwxyz", 17, 0));
EXPECT_EQ(TFW_STR_CHUNKN(&out), 3);

tfw_str_collect_cmp(chunks + 3, chunks + 5, &out, NULL);
EXPECT_TRUE(tfw_str_eq_cstr(&out, "rstuvwxyz", 9, 0));
EXPECT_EQ(TFW_STR_CHUNKN(&out), 2);

tfw_str_collect_cmp(chunks + 4, chunks + 5, &out, NULL);
EXPECT_TRUE(tfw_str_eq_cstr(&out, "uvwxyz", 6, 0));
/*
* Cutting out one segment should create a plain string, rather than
* a chunked one with a single segment.
*/
EXPECT_TRUE(TFW_STR_PLAIN(&out));

/* Empty slice. */
tfw_str_collect_cmp(chunks + 4, chunks + 4, &out, NULL);
EXPECT_TRUE(tfw_str_eq_cstr(&out, "", 0, 0));
EXPECT_TRUE(TFW_STR_PLAIN(&out));

/* Collecting until a stop character. Two chunks. */
tfw_str_collect_cmp(chunks, chunks + 5, &out, "j");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, also add a case, when stop character couldn't be found at the beginning of a new chunk, so resulting string won't overcome end limit. This is checked in tests above, but not for the case when stop character is provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

EXPECT_TRUE(tfw_str_eq_cstr(&out, "abcdefghi", 9, 0));
EXPECT_EQ(TFW_STR_CHUNKN(&out), 2);

/* Collecing until a stop character. Single chunk. */
tfw_str_collect_cmp(chunks + 1, chunks + 5, &out, "j");
EXPECT_TRUE(tfw_str_eq_cstr(&out, "efghi", 5, 0));
EXPECT_TRUE(TFW_STR_PLAIN(&out));

/*
* tfw_str_collect_cmp() is expected to check for the stop character
* only at the beginning of each segment. Even if the character appears
* somewhere inside, all segments are expected to be collected.
*/
tfw_str_collect_cmp(chunks, chunks + 5, &out, "k");
EXPECT_TRUE(tfw_str_eq_cstr(&out, "abcdefghijklmnopqrstuvwxyz", 26, 0));
EXPECT_EQ(TFW_STR_CHUNKN(&out), 5);
}

TEST_SUITE(tfw_str)
{
TEST_SETUP(create_str_pool);
Expand Down Expand Up @@ -1155,4 +1233,5 @@ TEST_SUITE(tfw_str)
TEST_RUN(tfw_str_eq_cstr_off, compound);

TEST_RUN(tfw_str_crc32, plain_compound);
TEST_RUN(tfw_str_collect_cmp, collect_chunks);
}