Hi, On Mon, Sep 16, 2024 at 11:57 AM Celeste Liu <coelacanthushex@xxxxxxxxx> wrote: > > > 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? Afaik VLA has more problems than using alloca, because depending on the C (11-23) version VLA is optional, so afaik alloca is a safer option provided the length is not considered too big, anyway perhaps we should use some define like LINE_MAX instead: {LINE_MAX} Unless otherwise noted, the maximum length, in bytes, of a utility's input line (either standard input or another file), when the utility is described as processing text files. The length includes room for the trailing <newline>. Minimum Acceptable Value: {_POSIX2_LINE_MAX} https://www.man7.org/linux/man-pages/man0/limits.h.0p.html So something like the following: diff --git a/monitor/packet.c b/monitor/packet.c index c2599fe6864a..32a440bbea68 100644 --- a/monitor/packet.c +++ b/monitor/packet.c @@ -26,6 +26,7 @@ #include <time.h> #include <sys/time.h> #include <sys/socket.h> +#include <limits.h> #include "lib/bluetooth.h" #include "lib/uuid.h" @@ -376,7 +377,7 @@ 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 line[LINE_MAX], ts_str[96], pid_str[140]; int n, ts_len = 0, ts_pos = 0, len = 0, pos = 0; static size_t last_frame; > > > >> > >>> 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> > >>> > >>> > >> > >> > -- Luiz Augusto von Dentz
