RFR(S): 8214854: JDWP: Unforseen output truncation in logging

[David Holmes]

Looks good.

Thanks,
David

On 2/03/2019 7:46 am, Dmitry Chuyko wrote:
> Ok. typedef part removed according to review comments (not related to fix).
> 
> Updated webrev: http://cr.openjdk.java.net/~dchuyko/8214854/webrev.01/
> 
> -Dmitry
> 
> On 3/1/19 10:07 PM, Chris Plummer wrote:
>> On 2/28/19 11:12 PM, Dmitry Chuyko wrote:
>>> 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).
>> Hi Dmitry,
>>
>> Ok, but that's unrelated to the compiler warning and not needed for 
>> the fix. Using a typedef hides the fact that we are dealing with a 
>> char* so makes the code less readable. I actually prefer the way it 
>> was with a char* and size_t arguments. My next preference would be:
>>
>>      get_time_stamp(char tbuf[MAXLEN_TIMESTAMP+1])
>>
>> Although I think it's also a bit odd. I don't recall seeing array 
>> sizes in function prototypes before.
>>
>> thanks,
>>
>> Chris
>>
>>> 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
>>>>>>
>>>>
>>>>
>>
>>