On 2024-09-17 00:22, Luiz Augusto von Dentz wrote: > 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 v3 has been sent. > 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> >>>>> >>>>> >>>> >>>> >> > >
