[genivi-dlt] [PATCH] All dlt_user functions return DLT_RETURN_BUFFER_FULL when, buffer full

Helwing, Lutz Lutz_Helwing at mentor.com
Tue Mar 22 09:40:57 EDT 2016


Hi all,

as promised i've splitted the change into three patches.

0001-Revert-truncation-of-string-or-raw-block.patch:
Commit d319ebf4d2d449d70befae0eebbdd82c19c5315f introduced new handling 
of the situation when writing data to the user buffer would exceed its 
size. This was achieved by truncating the data to fit. This solution had 
a problem:

It could lead to a buffer overflow when the updated arg_size gets 
negative (actually undefined value because it is unsigned).

As a quick fix the behaviour is reverted to the state before the 
aforementioned commit.


0002-DLT_RETURN_USER_BUFFER_FULL-is-returned-when-user-bu.patch:
A new return value DLT_RETURN_USER_BUFFER_FULL is introduced which is 
returned by all dlt_user_log_write_* and dlt_log_* functions when the 
data to be added to the user buffer exceeds its size (datasize > 
DLT_USER_BUF_MAX_SIZE). This enables users of these functions to send 
the full buffer and the write their data again to a yet empty buffer so 
no data gets lost.

This affects all calls of these functions which compare their return 
value with "== DLT_RETURN_ERROR". When looking into the dlt-daemon code 
itself i've found no occurrence of this construct. All calls which do 
checks do it with "< DLT_RETURN_OK" which  is ok.


0003-Added-abnormal-unit-tests-to-check-DLT_RETURN_USER_B.patch:
Checks for DLT_RETURN_USER_BUFFER_FULL added


Another task i would recommend is to have a closer look at all functions 
that check for DLT_RETURN_ERROR by using ==. Some time ago checks for 
invalid parameters were introduced which return 
DLT_RETURN_WRONG_PARAMETER for example when a pointer parameter is NULL 
where it mustn't be. All mentioned checks do not cover this case. I have 
found 83 such occurrences within the dlt-daemon source tree. 
Unfortunately due to project load and upcoming parental leave i will not 
be able to look through this now.

I'd like to mention that these patches are proposals thus it would be 
nice if there were more people like Gunnar who do take a look at patches 
and trigger discussions.

Best regards
Lutz

P.S. as you can see
# file 000*.patch
0001-Revert-truncation-of-string-or-raw-block.patch: unified diff 
output, ASCII text
0002-DLT_RETURN_USER_BUFFER_FULL-is-returned-when-user-bu.patch: unified 
diff output, ASCII text
0003-Added-abnormal-unit-tests-to-check-DLT_RETURN_USER_B.patch: unified 
diff output, ASCII text
do not contain any Windows line endings

On 18/03/16 18:19, Helwing, Lutz wrote:
> Hello Gunnar,
>
> thanks for your remarks. Don't worry: of course i'm not offended :)
>
> Some points:
>
> The behaviour of the interface can and should be discussed, that's
> right. If anyone did other checks for return values than ret !=
> DLT_RETURN_OK there would be an impact through this patch. The truncate
> "fix" also meant a change in behaviour of DLT which no one seemed to
> have a problem with. The quick fix changes behaviour again.
>
> The whitespace mess came from reverting to the old state, i'm sorry, i
> should have been more selective in reverting. The weird long lines can
> be seen all over the file in the parts i did not change. All in all,
> frankly speaking, the DLT code is a huge mess when looking at
> formatting. The coding style even changes within the same file and
> sometimes even within the same function. I cannot see compliance to any
> coding guidelines at all. Where can the "DLT coding standard" be found?
> Would be interesting to have a look at it. For the Windows line endings
> i must say that i don't see any in the patch file i have on my computer
> and it would be strange because i don't use Windows. I will cross-check
> this again.
>
> One reason for things not being caught in unit tests is obvious: no one
> contributes any.
>
> All in all it would be a very worthwhile project to raise the DLT code
> format quality to some acceptable level and to massively work on
> improvement of testing. If DLT quality really is pretty important to a
> lot of companies it should be a doable effort.
>
> I will check the patch and re-submit after splitting the commits.
>
> Thanks again for looking at the patch.
>
> Kind regards
> Lutz
>
> On 18/03/16 17:34, Andersson, Gunnar wrote:
>> Hello Lutz
>>
>> First time reviewing these patches so a lot of spontaneous
>> reactions, no offense, but DLT quality is pretty important
>> to a lot of companies.
>>
>> To be direct, the patch looks like a big mess.  Can you separate
>> the following changes, all of which seem to be included at the
>> same time here:
>>
>> 1. Quick fix needed for crashing(?) bug that causes buffer
>> overflow as described below.
>>
>> 2. Changing the return value behavior of public interfaces...
>> There is a lot of DLT code out there nowadays depending on those
>> interfaces.  I realize this is development and not a tagged
>> release yet, but I'd be a whole lot more careful about changes
>> like this - besides you state yourself it may not be the right
>> solution yet.  So, it sounds like it needs more discussion then?
>>
>> 3. Whitespace changes all over the place.  I don't know if this
>> long line with no spaces between any tokens is according to
>> DLT code standard or not...
>>
>>> +if ((log-
>>> size+length+sizeof(uint32_t)+sizeof(uint16_t))>DLT_USER_BUF_MAX_SIZE)
>> ...but if it is then still put the many many such changes in a
>> separate commit, please.
>>
>> You can't expect people to review the critical parts of this when
>> the patch submission is this long and such a big mix of unrelated
>> things.
>>
>> (Also, did you mean to have Windows line feeds on the whole patch?
>> I guess I can't say anything about the cross-platform dlt-viewer but
>> in dlt-daemon I think it has no place.)
>>
>> The previous faulty commit [1] was smaller but it suffered from
>> similar issues (whitespace changes and stuff), which makes it more
>> difficult to nail down where the mistake was really made and I think
>> also more difficult to just revert to a working solution instead of
>> what here seems like another stressed solution (possibly creating
>> more problems?)
>>
>> I would recommend some analysis of why this was not caught in unit
>> tests.  A test was commented out in last commit - did you explain
>> why?
>>
>>   From me it would be a -1 and please resubmit separate patches.
>> No offense.
>>
>> Best Regards
>> - Gunnar
>> [1] http://git.projects.genivi.org/?p=dlt-daemon.git;a=commitdiff;h=d319ebf4
>> d2d449d70befae0eebbdd82c19c5315f;hp=858e87c83bc527a546ee7ec3617f2a306a5e45fc
>>
>>
>>
>> On Fri, 2016-03-18 at 15:12 +0000, Helwing, Lutz wrote:
>>> Dear DLT community,
>>>
>>> we have found an issue with truncation of messages which do not fit into
>>> buffer or when buffer is full. Thus we would like to propose a patch that
>>> will undo the change in some way:
>>>
>>> Commit d319ebf4d2d449d70befae0eebbdd82c19c5315f introduced new handling of
>>> the situation when writing data to the user buffer would exceed its size.
>>> This was achieved by truncating the data to fit. This solution had two
>>> problems:
>>>
>>> 1. It contained a bug which could lead to a buffer overflow when the
>>> updated arg_size gets negative (actually undefined value because it is
>>> unsigned).
>>>
>>> 2. Possibly useful data get lost with truncation.
>>>
>>> Thus the patch changes the behaviour of the DLT library functions to
>>> return DLT_RETURN_BUFFER_FULL instead of truncating. This enables the user
>>> to trigger a send of the buffer and then trying to write the same data to
>>> the now empty buffer again so nothing is lost.
>>>
>>> Also some tests have been implemented to test if the correct value is
>>> returned when too much data is tried to be written.
>>>
>>> Perhaps this way of handling the "buffer full" situation is also not that
>>> brilliant. It would be nice to discuss a way to handle this more more
>>> reasonably. In the best case by the library itself?
>>>
>>> Kind regards
>>> Lutz
>>> -- 
>>>
>>> Lutz Helwing
>>> Senior Engineer
>>>    Telefon  	 +49 (89) 57096 - 297
>>>    Fax 	 +49 (89) 57096 - 400
>>>    lutz_helwing at mentor.com
>>>    
>>> Mentor Graphics Development (Deutschland) GmbH
>>> Arnulfstrasse 201
>>> D-80634 München
>>> http://www.mentor.com
>>>    Mentor Graphics Development (Deutschland) GmbH
>>> Geschäftsführung: Dean Freed, Walter Vermijs, Shannon Wetzel
>>> Handelsregister: Amtsgericht Freiburg i.Br., HRB 705237
>>> Sitz der Gesellschaft: Peterzeller Straße 8, 78048 Villingen-Schwenningen
>>> USt-Id Nummer: DE268786819
>>>    Confidentiality Notice: This e-mail message, including any attachments,
>>> is for the sole use of the intended recipient(s) and may contain
>>> confidential and privileged information. Any unauthorized review, use,
>>> disclosure or distribution is prohibited. If you are not the intended
>>> recipient, please contact the sender by return e-mail and destroy all
>>> copies of the original message.
>>>    
>>> _______________________________________________
>>> genivi-diagnostic-log-and-trace mailing list
>>> genivi-diagnostic-log-and-trace at lists.genivi.org
>>> https://lists.genivi.org/mailman/listinfo/genivi-diagnostic-log-and-trace
> _______________________________________________
> genivi-diagnostic-log-and-trace mailing list
> genivi-diagnostic-log-and-trace at lists.genivi.org
> https://lists.genivi.org/mailman/listinfo/genivi-diagnostic-log-and-trace

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Revert-truncation-of-string-or-raw-block.patch
Type: text/x-patch
Size: 5330 bytes
Desc: 0001-Revert-truncation-of-string-or-raw-block.patch
URL: <http://lists.genivi.org/pipermail/genivi-diagnostic-log-and-trace_lists.genivi.org/attachments/20160322/a6252055/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-DLT_RETURN_USER_BUFFER_FULL-is-returned-when-user-bu.patch
Type: text/x-patch
Size: 17566 bytes
Desc: 0002-DLT_RETURN_USER_BUFFER_FULL-is-returned-when-user-bu.patch
URL: <http://lists.genivi.org/pipermail/genivi-diagnostic-log-and-trace_lists.genivi.org/attachments/20160322/a6252055/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Added-abnormal-unit-tests-to-check-DLT_RETURN_USER_B.patch
Type: text/x-patch
Size: 8144 bytes
Desc: 0003-Added-abnormal-unit-tests-to-check-DLT_RETURN_USER_B.patch
URL: <http://lists.genivi.org/pipermail/genivi-diagnostic-log-and-trace_lists.genivi.org/attachments/20160322/a6252055/attachment-0002.patch>


More information about the genivi-diagnostic-log-and-trace mailing list