On 2024-09-16 23:39, Celeste Liu wrote: > > 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. But I found a VLA usage in monitor/display.h:82, which was introduced by Marcel Holtmann in d9e3aab39d2af7d7a822993ededaa41cd0311c53 in 2012. Could we use VLA directly? Or we need to treat that usage as a bug and fix it? > >> >>> 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> >>> >>> >> >>
