Skip to content

lightningd: don't crash when truncating large log messages#9331

Open
whitslack wants to merge 1 commit into
ElementsProject:masterfrom
whitslack:log-msg-truncation-crash
Open

lightningd: don't crash when truncating large log messages#9331
whitslack wants to merge 1 commit into
ElementsProject:masterfrom
whitslack:log-msg-truncation-crash

Conversation

@whitslack

Copy link
Copy Markdown
Collaborator

It's not okay to call free() on the pointer to a truncated log message that was allocated by tal_fmt() in cap_header(). Let's call tal_free() instead, and rather than calling vasprintf() to malloc the log message in the first place, let's call tal_vfmt().

Also, since we're now always using a tallocated string for the log message, let's have cap_header() take ownership of it and either free it if it truncated the message (and is returning a different pointer to the truncated message) or else return the taken original pointer without freeing it.

Also, log_io()'s str and data parameters are marked TAKES, but the function was not actually taking them, so fix that up too.

Also, don't call strlen() on a string returned by tal_fmt(). The returned pointer is guaranteed to have tal_count() == strlen() + 1, so there's no sense in scanning through the string to find its length.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

  • The changelog has been updated in the relevant commit(s) according to the guidelines. N/A
  • Tests have been added or modified to reflect the changes. N/A
  • Documentation has been reviewed and updated as needed. N/A
  • Related issues have been listed and linked, including any that this PR closes. None found
  • Important All PRs must consider how to reverse any persistent changes for tools/lightning-downgrade N/A

It's not okay to call free() on the pointer to a truncated log message
that was allocated by tal_fmt() in cap_header(). Let's call tal_free()
instead, and rather than calling vasprintf() to malloc the log message in
the first place, let's call tal_vfmt().

Also, since we're now always using a tallocated string for the log message,
let's have cap_header() take ownership of it and either free it if it
truncated the message (and is returning a different pointer to the truncated
message) or else return the taken original pointer without freeing it.

Also, log_io()'s str and data parameters are marked TAKES, but the function
was not actually taking them, so fix that up too.

Also, don't call strlen() on a string returned by tal_fmt(). The returned
pointer is guaranteed to have tal_count() == strlen() + 1, so there's no
sense in scanning through the string to find its length.

Changelog-None
Comment thread lightningd/log.c
@@ -685,8 +684,8 @@ void logv(struct logger *log, enum log_level level,
l.time,
l.prefix->prefix,
logmsg);
free(logmsg);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is where the crash was occurring. The crash happens if cap_header() tallocates and returns a new string to hold the truncated message. Then we'll be trying to free() a tal pointer. Boom.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant