[genivi-dlt] [patch] check payload length before casting

Andersson, Gunnar gunnar.x.andersson at volvocars.com
Wed Dec 21 03:07:35 EST 2016


Hello everyone

I've had a little conversation with Christoph on this pull request.

At first I thought the repetitive error checking code in this module
(dlt_daemon_client.c) could quite easily be reduced to one place, kind of
like Christoph suggested later in the PR comments, a macro (or an inline
function).

So I wanted to fix it up quickly with Christoph so we could merge the
change in since I have limited time to put on code hacking right now
(too many other things to coordinate and a sizeable backlog as always).

But now I feel it wasn't that easy.  So I'd like to ask Alexander and
others some questions to bring this forward because I don't want to be
blocking Christoph's bug fix.

One of the things that trip me up is that checking the input parameters is
not done fully consistently.  Is this just by mistake, or is it delberate?
(A lot of times such small differences is exactly the kind of result that
 comes from not avoiding repetition...)


Could the maintainer(s) and experts please consider these questions 
and comment:

In dlt_daemon_client.c:

A) Is it necessary to check all input pointers, or only some?   For
example, daemon_local - should we check it for NULL? Only some 
functions check NULL for daemon_local pointer, which is confusing!

B) Who calls these functions?  Since clients of DLT make use of library
functions I first imagined all of these calls are "internal" to DLT but 
I'm not sure.   

For question A of course the defensive and safe way is to check all
pointers.  But what I'm asking is also if this is an assert() condition
or not...

I'm assuming DLT maintainers and most people have roughly the same idea of
software quality, but some of the code here does not fully agree with this,
so therefore let me elaborate:

I believe it's important to categorize error handling in different
categories.  For example:

1. Finding bugs in *this* software module, that is "your own code".
Basically this boils down to checking as much of our assumptions as
possible and failing very clearly if something turned out to not be true.
Crash early and often when it comes to your own bugs....  These are
situations we do *not* expect to ever happen, but could happen if we 
made a bug.

2. Handling input from others.  For example a library called by an unknown
program should expect any of its inputs to be incorrect and therefore check
these.  Compared to 1, this is an "expected" error condition.

3. "Robustness".  In particular in an embedded systems we may be fine to
crash early and often during development, but once we put them in
production we might not want to crash/exit.  At least if there is some
meaningful way that the program can continue then it's desired to handle
the error condition softly.  that may be in direct contrast to what we
want in order to find bugs...

Number 1 should translate to assert(condition).  I see very few
asserts in this code base (some in dlt_env_ll.c) and would recommend
everyone to use them more*.  An assert call tells a code reader that a
failed assertion is something we never expect to happen, but we want to be
alerted if it does anyway.   Asserts usually create a log and many times
also stop the program and require that we fix the code*.  It provokes
bugs to make them visible!

Number 2 should not translate to an assert but to:   if (error condition) {
} , and the function should handl it.  Return a predefined error and/or log the 
error.  In comparison to the assert, this tells a reader that it is a condition 
we expect could happen once in a while.

Number 3 is coded similar to number 2 but with extra careful consideration
about when it makes sense to handle an error softly and what the execution
might look like if we continue despite an error condition.

* The default implementation of an assert in a normal program is to print
out the failing file and line number to the console, and then exit the
program.  (In an embedded system the printout usually would be directed to
some log instead).  In production build the standard assert
implementation is to translate to nothing - they are disabled and have no
effect at all.   Note of course we may implement our own assert
differently: something that is appropriate for the development situation
as well as the production build.  A log and exit in development and perhaps
a soft log (ironically DLT is what we would use :-) but continue execution.

So I've tried a little to follow the code flow but I'm not 100% sure and
it's quicker to ask the experts.  If those of you who know this well could
tackle the questions A) and B), it should bring us forward.   For example,
is a wrong message buffer length a bug that is totally internal to the DLT
module, or is that something that an external client could cause to happen?


Best Regards
- Gunnar  

________________________________________
Från: genivi-diagnostic-log-and-trace [genivi-diagnostic-log-and-trace-bounces at mailman1.genivi.org] för Lipka, Christoph (ADITJ/SWG) [clipka at jp.adit-jv.com]
Skickat: den 30 november 2016 08:59
Till: genivi-diagnostic-log-and-trace at mailman1.genivi.org
Ämne: [genivi-dlt] [patch] check payload length before casting

Dear all,

I created a pull request to solve possible issues parsing control messages in case the actual received data is smaller than the structure size of the control message.
It can be found here: https://github.com/GENIVI/dlt-daemon/pull/3

The patch is also attached to this mail.

Best regards

Christoph Lipka
Advanced Driver Information Technology
Software Group (ADITJ/SWG)
1-1 Showa-cho, Kariya-shi
Aichi-ken 448-8661, Japan
Tel. +81-(0)566 61-5124
Fax +81-(0)566 25-4774
clipka at jp.adit-jv.com<mailto:clipka at jp.adit-jv.com>

ADIT is joint venture company of DENSO Corporation and Bosch GmbH




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