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

Remove 8K limit for single access.log line #332

Closed
wants to merge 12 commits into from
Closed

Remove 8K limit for single access.log line #332

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
045439f
Removed 8K limit for single log line.
dnevil Nov 14, 2018
35f3acd
Changes as requested for logfilePrintf(). Moved LOGFILE_BUFSZ to Fil…
dnevil Nov 15, 2018
8702c85
Removed thread_local declaration from logfilePrintf().
dnevil Nov 15, 2018
eb12621
Reverted move of LOGFILE_BUFSZ. Simplified code in logfilePrintf().
dnevil Nov 15, 2018
5f3a3eb
Sync prototype of logfileWrite() with definition.
dnevil Nov 15, 2018
3b64034
Merge branch 'master' into log-line-limit
redmeadowman Nov 16, 2018
8bc33c1
Minor change to include ordering.
dnevil Nov 27, 2018
35a46cc
Merge branch 'log-line-limit' of https://github.com/sisic-inc/squid i…
dnevil Nov 27, 2018
f8c25ef
Merge branch 'master' into log-line-limit
redmeadowman Nov 27, 2018
4ead5e0
Add TODO for overflow handling
yadij Feb 11, 2020
fc2501d
Merge branch 'master' into log-line-limit
rousskov Feb 12, 2020
0bdd5a7
Merge branch 'master' into log-line-limit
rousskov Feb 18, 2020
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
21 changes: 6 additions & 15 deletions src/log/File.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "log/ModSyslog.h"
#include "log/ModUdp.h"
#include "log/TcpLogger.h"
#include "sbuf/SBuf.h"

CBDATA_CLASS_INIT(Logfile);

Expand Down Expand Up @@ -103,7 +104,7 @@ logfileRotate(Logfile * lf, int16_t rotateCount)
}

void
logfileWrite(Logfile * lf, char *buf, size_t len)
logfileWrite(Logfile * lf, const char *buf, size_t len)
{
lf->f_linewrite(lf, buf, len);
}
Expand All @@ -112,21 +113,11 @@ void
logfilePrintf(Logfile * lf, const char *fmt,...)
{
va_list args;
char buf[8192];
int s;

va_start(args, fmt);

s = vsnprintf(buf, 8192, fmt, args);

if (s > 8192) {
s = 8192;

if (fmt[strlen(fmt) - 1] == '\n')
buf[8191] = '\n';
}

logfileWrite(lf, buf, (size_t) s);
static SBuf sbuf;
sbuf.clear();
sbuf.vappendf(fmt, args);
Copy link
Contributor

Choose a reason for hiding this comment

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

Other bugs notwithstanding, the maximum line size is now limited to 256 MB (SBuf::maxSize or 268435455 bytes to be precise). That is more than enough, of course, but I wonder whether we should add code to handle overflows. Without such code, an unlikely huge access.log line will either kill Squid or not appear in the access.log at all. Thus, we are back to the six overflow handling options discussed earlier, except that the simplest option 6 is off the table. Since we no longer deal with raw formatting code here, I suggest working around this problem instead of solving it directly:

  1. Do not handle overflows in logfilePrintf(). Just check that if SBuf throws, there will some ERROR message about that in cache.log. To check, temporary hack logfilePrintf() code to add Must(false) and see what happens -- do that before moving on to step2 below.
  2. Change Log::Format::SquidCustom() to print the already formatted(!!) line using logfileWrite() instead of abusing logfilePrintf(). You may need to refactor a little so that Format::assemble() appends a new line when needed.

The above approach is based on the assumption that the most likely (if not the only source) of logfilePrintf() overflows is custom log formats. And if we want to improve overflow handling there, we have to improve Format::assemble(), where overflow handling approaches like the nice option 5 posted earlier are much easier to implement. We do not have to do those improvements in this PR though -- the above two steps should be enough for now as far as overflow handling is concerned.

Copy link
Contributor

Choose a reason for hiding this comment

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

The above approach is based on the assumption that the most likely (if not the only source) of logfilePrintf() overflows is custom log formats.

Alex, that assumption is wrong. Anything that logs URL/URI and/or header field-values can produce very long lines. That means all log formats built into Squid are possible overflows. Format::assemble is also used in all the non-log places where logformat macros are supported (error pages, deny_ino URLs, external ACL helper I/O) - so care needs to be taken not to break any of those by forcing implicit line-wrapping into its output.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alex: the most likely source of logfilePrintf() overflows is custom log formats

Amos: that assumption is wrong. [...] all log formats built into Squid are possible overflows

Your statement does not contradict my assumption AFAICT, but, more importantly, I believe that the already implemented PR changes invalidate your statement: Some logfilePrintf() callers cannot overflow at all (even before the PR), and only custom log format has any chance of producing log lines that exceed 268435455 characters (the post-PR limit).

care needs to be taken not to break any of those by forcing implicit line-wrapping into Format::assemble() output

Yes, that is why I said "appends a new line when needed". One way to handle this without adding parameters/options to assemle() could be to append a newline to the configured logformat string.

Copy link
Contributor

Choose a reason for hiding this comment

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

This change request is still unresolved. The two-step plan I have suggested still applies AFAICT.

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 am out of time on this project. I will not implement the suggested change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will not implement the suggested change.

Thank you for sharing your decision. I believe the requested change should be implemented before this PR is approved because it is not difficult to implement, will address a potentially important in-scope problem, and even provide a significant performance improvement. Hopefully, somebody else will take over and finish this work.

However, if other @squid-cache/commiters disagree, they may be able to approve this PR (after a thorough from-scratch review) without this change because they may decide that the PR is a step forward, while that known problem is unlikely to surface in real deployments.

rousskov marked this conversation as resolved.
Show resolved Hide resolved
logfileWrite(lf, sbuf.c_str(), sbuf.length());
rousskov marked this conversation as resolved.
Show resolved Hide resolved
va_end(args);
}

Expand Down
2 changes: 1 addition & 1 deletion src/log/File.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class Logfile
Logfile *logfileOpen(const char *path, size_t bufsz, int);
void logfileClose(Logfile * lf);
void logfileRotate(Logfile * lf, int16_t rotateCount);
void logfileWrite(Logfile * lf, char *buf, size_t len);
void logfileWrite(Logfile * lf, const char *buf, size_t len);
void logfileFlush(Logfile * lf);
void logfilePrintf(Logfile * lf, const char *fmt,...) PRINTF_FORMAT_ARG2;
void logfileLineStart(Logfile * lf);
Expand Down