log: fix potential overflow with long log messages #490
Conversation
qb_vsnprintf_serialize was called with 'max_size' as the limiting number for the length of the formatted log message. But the buffer also needs to contain the log header (given by 'actual_size'), so we now pass 'max_size - actual_size' as the maximum length of the formatted log message. Also added error checks to the blacbkbox calls at the end of the test, as these now provide a proper test that the BB is functioning. Before they were masking failures.
chrissie-c
force-pushed
the
fix-log-overflow
branch
from
ae0eeeb
to
2c16eb9
Compare
https://build.opensuse.org/request/show/1100598 by user yan_gao + anag+factory - Update to version 2.0.8+20230721.002171b (v2.0.8): - log: fix potential overflow with long log messages (gh#ClusterLabs/libqb#490) (forwarded request 1100597 from yan_gao)
| msg_len = qb_vsnprintf_serialize(chunk, max_size, cs->format, ap); | ||
| if (msg_len >= max_size) { | ||
| msg_len = qb_vsnprintf_serialize(chunk, t->max_line_length, cs->format, ap); | ||
| if (msg_len >= t->max_line_length) { | ||
| chunk = msg_len_pt + sizeof(uint32_t); /* Reset */ | ||
|
|
||
| /* Leave this at QB_LOG_MAX_LEN so as not to overflow the blackbox */ |
There was a problem hiding this comment.
FYI, the next line could theoretically be a problem if the caller used QB_LOG_CONF_MAX_LINE_LEN to set the max line length below the length of the "too long" message. (I wouldn't consider it a security issue since the message is hardcoded, not to mention it would be bizarre for a caller to do that). In any case, the "too long" message isn't really helpful -- why not just use as much of the long message as possible (maybe replacing the last three usable characters with an ellipsis)?
There was a problem hiding this comment.
+1 for replacing "too long" message with shortened version of logged message. Also I think it may make sense to check QB_LOG_CONF_MAX_LINE_LEN for some minimal reasonable length (right now only maximum length of QB_LOG_ABSOLUTE_MAX_LEN is checked)
|
The ellipsis code has been in there for ages, but it needs to be enabled: |
log_blackbox.c doesn't use qb_log_target_format() (I'm not familiar enough with the code to know why not), so the ellipsis code isn't hit |
qb_vsnprintf_serialize was called with 'max_size' as the limiting number for the length of the formatted log message. But the buffer also needs to contain the
log header (given by 'actual_size'), so we now pass 'max_size - actual_size' as the maximum length of the formatted log message.
Also added error checks to the blacbkbox calls at
the end of the test, as these now provide a proper test that the BB is functioning. Before they were
masking failures.