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

[Dmitry Chuyko]

Thank you for reviews. Pushed.

-Dmitry

On 3/2/19 2:28 AM, Chris Plummer wrote:
> +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
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>
>