Re: [PATCH bluez v2] monitor: fix buffer overflow when terminal width > 255

[Celeste Liu <coelacanthushex@gmail.com>]

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@gmail.com> 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@gmail.com>
>> ---
>> Changes in v2:
>> - Add free() forgot in v1.
>> - Link to v1: https://patch.msgid.link/20240914-fix-log-buffer-overflow-v1-1-733cb4fff673@gmail.com
>> ---
>>  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@gmail.com>
>>
>>
> 
>