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

[Celeste Liu <coelacanthushex@gmail.com>]

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@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.

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@gmail.com>
>>>
>>>
>>
>>