On 2024-09-16 23:10, Luiz Augusto von Dentz wrote: > Hi Celeste, > > On Sat, Sep 14, 2024 at 12:10 PM Celeste Liu <coelacanthushex@xxxxxxxxx> wrote: >> >> In current code, we create line buffer with size 256, which can contains >> 255 ASCII characters. But in modern system, terminal can have larger >> width. It may cause buffer overflow in snprintf() text. >> >> We need allocate line buffer with size which can contains one line in >> terminal. The size should be difficult to calculate because of multibyte >> characters, but our code using line buffer assumed all characters has >> 1 byte size (e.g. when we put packet text into line buffer via >> snprintf(), we calculate max size by 1B * col.), so it's safe to >> allocate line buffer with col + 1. >> >> Signed-off-by: Celeste Liu <CoelacanthusHex@xxxxxxxxx> >> --- >> Changes in v2: >> - Add free() forgot in v1. >> - Link to v1: https://patch.msgid.link/20240914-fix-log-buffer-overflow-v1-1-733cb4fff673@xxxxxxxxx >> --- >> monitor/packet.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/monitor/packet.c b/monitor/packet.c >> index c2599fe6864ab44d657c121fcc3ceecc1ebc52a6..bef55477a221b6cb43ff224454ac3fa593cd8221 100644 >> --- a/monitor/packet.c >> +++ b/monitor/packet.c >> @@ -376,7 +376,8 @@ static void print_packet(struct timeval *tv, struct ucred *cred, char ident, >> const char *text, const char *extra) >> { >> int col = num_columns(); >> - char line[256], ts_str[96], pid_str[140]; >> + char ts_str[96], pid_str[140]; >> + char *line = (char *) malloc(sizeof(char) * col + 1); > > Perhaps we could replace malloc with alloca here so we allocate the > line on the heap rather than stack. I will replace it with alloca() in the next version. But to be honest, I think alloca() is not a good choice. The compiler will prevent the functions that call alloca() be inline, otherwise it may trigger unexpected stack overflow because it's not a scope-based lifetime. It may be better to replace it with VLA once we bump the standard requirement to C99 or above. > >> int n, ts_len = 0, ts_pos = 0, len = 0, pos = 0; >> static size_t last_frame; >> >> @@ -525,6 +526,7 @@ static void print_packet(struct timeval *tv, struct ucred *cred, char ident, >> printf("%s%s\n", use_color() ? COLOR_TIMESTAMP : "", ts_str); >> } else >> printf("%s\n", line); >> + free(line); >> } >> >> static const struct { >> >> --- >> base-commit: 41f943630d9a03c40e95057b2ac3d96470b9c71e >> change-id: 20240914-fix-log-buffer-overflow-9aa5e61ee5b8 >> >> Best regards, >> -- >> Celeste Liu <CoelacanthusHex@xxxxxxxxx> >> >> > >
