RFR(S): 8214854: JDWP: Unforseen output truncation in logging
Dmitry Chuyko
dmitry.chuyko at bell-sw.com
Fri Mar 1 07:12:51 UTC 2019
On 3/1/19 6:36 AM, Chris Plummer wrote:
> On 2/28/19 5:07 PM, David Holmes wrote:
>> Hi Dmitry,
>>
>> On 1/03/2019 6:15 am, Dmitry Chuyko wrote:
>>> Hello,
>>>
>>> Please review a small fix for GCC 8.x warning in log_messages.c.
>>> Buffer sizes for parts of timestamp string can be adjusted to not
>>> exceed maximum buffer size when combined, while being able to hold
>>> necessary information.
>>>
>>> webrev: http://cr.openjdk.java.net/~dchuyko/8214854/webrev.00/
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8214854
>>> testing: after the fix GCC 8 compiles working OpenJDK on Linux.
>>> Dev-submit job started.
>>
>> This looks good.
>>
>> One query:
>>
>> 51 typedef char timestamp_buffer[TIMESTAMP_SIZE];
>>
>> Is this needed so that the size of the buffer is known by gcc inside
>> get_time_stamp? (Generally I prefer to see explicitly when an array
>> is being stack allocated, not have it hidden behind a typedef.)
> This also caught my attention and then got me thinking on what basis
> is gcc producing the warning in the first place. The warning is:
>
> src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c:75:24: error:
> '%.3d' directive output may be truncated writing between 3 and 11
> bytes into a region of size between 0 and 80 [-Werror=format-truncation=]
> "%s.%.3d %s", timestamp_prefix,
> ^~~~
>
> I think it deduced this by seeing that the call to get_time_stamp() is
> passing in a char[80] for the tbuf argument. It then also sees that
> the argument for the first %s is also a char[80] array, so is pointing
> out that this first %s might fill (or nearly fill) tbuf, not leaving
> room for the %.3d argument. If this is the case, there is no need for
> the timestamp_buffer typedef. The real fix was making
> timestamp_date_time and timestamp_timezone smaller so gcc doesn't
> think they can overflow tbuf.
>
> Chris
Current signature of helper function is get_time_stamp(char *tbuf,
size_t ltbuf). If we drop typedef change, this function either logically
needs ltbuf parameter or it internally assumes it to be somewhat
(TIMESTAMP_SIZE+1). In first case we in general should care about
arbitrary ltbuf size. Like 5 or 2. In second case there are no static
checks/hints that we don't pass tbuf of diferent size. So I preferred to
have this information in the function signature and to use known lengths
of parts: get_time_stamp(timestamp_buffer tbuf).
-Dmitry
>
>
>>
>> Thanks,
>> David
>>
>>
>>> -Dmitry
>>>
>
>
More information about the serviceability-dev
mailing list