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

[Chris Plummer]

+1

thanks,

Chris

On 3/1/19 2:56 PM, David Holmes wrote:
> 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
>>>>>>>
>>>>>
>>>>>
>>>
>>>