Skip to content

LogAccessHttp init strlen to 0, not -1 (master) #2102

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

Merged
SolidWallOfCode merged 1 commit into from
Jun 14, 2017
Merged

LogAccessHttp init strlen to 0, not -1 (master) #2102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions proxy/logging/LogAccess.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@
#include "LogBuffer.h"
#include "Log.h"

char INVALID_STR[] = "!INVALID_STR!";

#define LOG_ACCESS_DEFAULT_FIELD(name, impl) \
int LogAccess::name(char *buf) { impl; }
/*-------------------------------------------------------------------------
Expand Down
2 changes: 2 additions & 0 deletions proxy/logging/LogAccess.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@
#define DEFAULT_STR "-"
#define DEFAULT_STR_LEN 1

extern char INVALID_STR[];

#define DEFAULT_INT_FIELD \
{ \
if (buf) { \
Expand Down
52 changes: 21 additions & 31 deletions proxy/logging/LogAccessHttp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,17 @@ LogAccessHttp::LogAccessHttp(HttpSM *sm)
m_client_req_url_canon_str(nullptr),
m_client_req_url_canon_len(0),
m_client_req_unmapped_url_canon_str(nullptr),
m_client_req_unmapped_url_canon_len(-1),
m_client_req_unmapped_url_canon_len(0),
m_client_req_unmapped_url_path_str(nullptr),
m_client_req_unmapped_url_path_len(-1),
m_client_req_unmapped_url_path_len(0),
m_client_req_unmapped_url_host_str(nullptr),
m_client_req_unmapped_url_host_len(-1),
m_client_req_unmapped_url_host_len(0),
m_client_req_url_path_str(nullptr),
m_client_req_url_path_len(0),
m_proxy_resp_content_type_str(nullptr),
m_proxy_resp_content_type_len(0),
m_cache_lookup_url_canon_str(nullptr),
m_cache_lookup_url_canon_len(-1)
m_cache_lookup_url_canon_len(0)
{
ink_assert(m_http_sm != nullptr);
}
Expand Down Expand Up @@ -269,7 +269,7 @@ LogAccessHttp::marshal_cache_lookup_url_canon(char *buf)
int len = INK_MIN_ALIGN;

validate_lookup_url();
if (0 >= m_cache_lookup_url_canon_len) {
if (m_cache_lookup_url_canon_str == INVALID_STR) {
// If the lookup URL isn't populated, we'll fall back to the request URL.
len = marshal_client_req_url_canon(buf);
} else {
Expand Down Expand Up @@ -326,7 +326,10 @@ LogAccessHttp::marshal_client_auth_user_name(char *buf)
void
LogAccessHttp::validate_unmapped_url()
{
if (m_client_req_unmapped_url_canon_len < 0) {
if (m_client_req_unmapped_url_canon_str == nullptr) {
// prevent multiple validations
m_client_req_unmapped_url_canon_str = INVALID_STR;

if (m_http_sm->t_state.unmapped_url.valid()) {
int unmapped_url_len;
char *unmapped_url = m_http_sm->t_state.unmapped_url.string_get_ref(&unmapped_url_len);
Expand All @@ -335,8 +338,6 @@ LogAccessHttp::validate_unmapped_url()
m_client_req_unmapped_url_canon_str =
LogUtils::escapify_url(&m_arena, unmapped_url, unmapped_url_len, &m_client_req_unmapped_url_canon_len);
}
} else {
m_client_req_unmapped_url_canon_len = 0;
}
}
}
Expand All @@ -352,10 +353,12 @@ LogAccessHttp::validate_unmapped_url_path()
int len;
char *c;

if (m_client_req_unmapped_url_path_len < 0 && m_client_req_unmapped_url_host_len < 0) {
if (m_client_req_unmapped_url_path_str == nullptr && m_client_req_unmapped_url_host_str == nullptr) {
// Use unmapped canonical URL as default
m_client_req_unmapped_url_path_str = m_client_req_unmapped_url_canon_str;
m_client_req_unmapped_url_path_len = m_client_req_unmapped_url_canon_len;
// Incase the code below fails, we prevent it from being used.
m_client_req_unmapped_url_host_str = INVALID_STR;

if (m_client_req_unmapped_url_path_len >= 6) { // xxx:// - minimum schema size
c = (char *)memchr((void *)m_client_req_unmapped_url_path_str, ':', m_client_req_unmapped_url_path_len - 1);
Expand Down Expand Up @@ -385,16 +388,17 @@ LogAccessHttp::validate_unmapped_url_path()
void
LogAccessHttp::validate_lookup_url()
{
if (m_cache_lookup_url_canon_len < 0) {
if (m_cache_lookup_url_canon_str == nullptr) {
// prevent multiple validations
m_cache_lookup_url_canon_str = INVALID_STR;

if (m_http_sm->t_state.cache_info.lookup_url_storage.valid()) {
int lookup_url_len;
char *lookup_url = m_http_sm->t_state.cache_info.lookup_url_storage.string_get_ref(&lookup_url_len);

if (lookup_url && lookup_url[0] != 0) {
m_cache_lookup_url_canon_str = LogUtils::escapify_url(&m_arena, lookup_url, lookup_url_len, &m_cache_lookup_url_canon_len);
}
} else {
m_cache_lookup_url_canon_len = 0;
}
}
}
Expand Down Expand Up @@ -527,7 +531,7 @@ LogAccessHttp::marshal_client_req_unmapped_url_canon(char *buf)
int len = INK_MIN_ALIGN;

validate_unmapped_url();
if (0 >= m_client_req_unmapped_url_canon_len) {
if (m_client_req_unmapped_url_canon_str == INVALID_STR) {
// If the unmapped URL isn't populated, we'll fall back to the original
// client URL. This helps for example server intercepts to continue to
// log the requests, even when there is no remap rule for it.
Expand All @@ -553,7 +557,7 @@ LogAccessHttp::marshal_client_req_unmapped_url_path(char *buf)
validate_unmapped_url();
validate_unmapped_url_path();

if (0 >= m_client_req_unmapped_url_path_len) {
if (m_client_req_unmapped_url_path_str == INVALID_STR) {
len = marshal_client_req_url_path(buf);
} else {
len = round_strlen(m_client_req_unmapped_url_path_len + 1); // +1 for eos
Expand All @@ -570,29 +574,15 @@ LogAccessHttp::marshal_client_req_unmapped_url_path(char *buf)
int
LogAccessHttp::marshal_client_req_unmapped_url_host(char *buf)
{
int plen = INK_MIN_ALIGN;

validate_unmapped_url();
validate_unmapped_url_path();

int alen = m_client_req_unmapped_url_host_len;
if (alen < 0) {
alen = 0;
}

// calculate the the padded length only if the actual length
// is not zero. We don't want the padded length to be zero
// because marshal_mem should write the DEFAULT_STR to the
// buffer if str is nil, and we need room for this.
if (alen) {
plen = round_strlen(alen + 1); // +1 for eos
}

int len = round_strlen(m_client_req_unmapped_url_host_len + 1); // +1 for eos
if (buf) {
marshal_mem(buf, m_client_req_unmapped_url_host_str, alen, plen);
marshal_mem(buf, m_client_req_unmapped_url_host_str, m_client_req_unmapped_url_host_len, len);
}

return plen;
return len;
}

int
Expand Down
4 changes: 3 additions & 1 deletion proxy/logging/LogField.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,9 @@ LogFieldList::marshal_len(LogAccess *lad)
int bytes = 0;
for (LogField *f = first(); f; f = next(f)) {
if (f->type() != LogField::sINT) {
bytes += f->marshal_len(lad);
const int len = f->marshal_len(lad);
Copy link
Contributor

@bryancall bryancall Jun 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't adding a check on len > 0 here and not add it to bytes also work? This way you wouldn't have to set m_cache_lookup_url_canon_str or have INVALID_STR.

Copy link
Collaborator Author

@a-canary a-canary Jun 8, 2017

Choose a reason for hiding this comment

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

The core of the problem was LogAccessHttp.cc:ln487,
len = round_strlen(m_client_req_unmapped_url_host_len + 1); // +1 for eos
When m_client_req_unmapped_url_host_len is -1, len is set to 0.
And strlen of -1 is kind of gross way to indicate that can't resolve the host name. In the future we would like to change all these to StringView, so this nudges us in that direction.

This assert is just to ensure no other code paths where having the same issue.

Copy link
Contributor

@bryancall bryancall Jun 9, 2017

Choose a reason for hiding this comment

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

I am not seeing that in the code on master round_strlen(m_client_req_unmapped_url_host_len + 1);. What line are you referring to?

Copy link
Contributor

@bryancall bryancall Jun 9, 2017

Choose a reason for hiding this comment

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

It looks like you are referring to the ATS 5.3.x branch in your comment above. This looks like it has been resolved on master and 7.1.x. Please confirm.

Copy link
Member

@SolidWallOfCode SolidWallOfCode Jun 9, 2017

Choose a reason for hiding this comment

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

Looks like commit 9399a76 patched this particular place (for TS-3841).

Copy link
Member

@SolidWallOfCode SolidWallOfCode Jun 9, 2017

Choose a reason for hiding this comment

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

I'm not sure what you're suggesting, Bryan, is different from the ink_release_assert on line 776 here. len shouldn't just be greater than zero, it should be at least INK_MIN_ALIGN.

Copy link
Contributor

@bryancall bryancall Jun 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I am stating this is not an issue for master and 7.1.x, if you want to add an assert that is OK, but is it not required. I would keep what is in LogField.cc and get rid of the rest of the changes.

Copy link
Member

@SolidWallOfCode SolidWallOfCode Jun 10, 2017

Choose a reason for hiding this comment

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

This is not the only logging anomaly we are seeing so I suspect it is in fact an issue elsewhere. I would rather do a basic fix rather than patching after the fact.

ink_release_assert(len >= INK_MIN_ALIGN);
bytes += len;
}
}
return m_marshal_len + bytes;
Expand Down