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

Andersson, Gunnar gunnar.x.andersson at volvocars.com
Fri Mar 18 12:34:24 EDT 2016


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


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