util/log: logic that breaks long lines also breaks redaction markers

[knz]

Found while inspecting cockroach demo --vmodule=exec_log=2:

I210507 20:49:42.762040 1905 9@event_log.go:32 ⋮ [...] 279 |... "‹'2018-12-16 03:04:05'›","<E2>
                                     first byte of a 3-byte UTF-8 sequence for "‹"        ^^^^

I210507 20:49:42.762040 1905 9@event_log.go:32 ⋮ [...] 279 |<80><B9>'2019-01-04 03:04:05'›"
           remaining 2 bytes of the 3-byte UTF-8 sequence ^^^^^^^^

This may qualify for a GA blocker too? Unsure

This is a deep 🤦 moment, and I wish I had listened to Andrei earlier...

[knz]

This may qualify for a GA blocker too? Unsure

@bdarnell says yes:

probably, since we'd have to disclose this as a security issue if we fix it in .1

[knz]

There are two avenues to address this:

1. prevent line breaks in the middle of UTF-8 sequences
2. prevent line breaks at non-newlines altogether

Pro/con analysis

• solution 1:
  • pros: directly addresses the issue at top. No regressions.
  • cons: slightly more complex than solution 2
• solution 2:
  • pros: fixes the issue at top; simpler than solution 1.
  • would cause two regressions:
    • it will un-fix log: log lines collected by debug.zip seem to be truncated to 65K #50166
    • it will break cockroach debug merge-log again when there are long entries in the input files
      (This issue was known to tech support but had not been filed on github, hence the lack of issue number)

@bdarnell suggests to use solution 2, and also simultaneously construct a different fix for #50166 and the merge-log issue: implement a new log entry parser. This would also address #61681.

We can call this "solution 3": Solution 2 + more unrelated work.

Pro/con analysis:

• solution 3:
  • pros: fixes the issue at top. Prevents a regression on log: log lines collected by debug.zip seem to be truncated to 65K #50166
  • cons:
    • more work overall than either solution 1 & 2
    • higher chance to introduce other regressions in the log parser, thereby impairing our ability to collect zip files
    • higher chance to blow up nodes with OOM crashes when log files contain many large stack dumps, since the entry parser would then stop truncating the entries.

[knz]

with this analysis, it seems like we should split the work as follows:

• for right now and 21.1.0, don't be too creative with the entry parser and just avoid splitting in the middle of runes (solution 1)
• discuss the longer-term fix (solution 2/3 or something else entirely for log: long-ish line broken up in multiple messages #61681) for a patch in 21.1.x

[bdarnell]

with this analysis, it seems like we should split the work as follows:

Agreed

avoid splitting in the middle of runes

Ideally this would be grapheme clusters instead of runes, but this would require a new third-party dependency (golang/go#14820). Runes are fine for now.