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

Lipka, Christoph (ADITJ/SWG) clipka at jp.adit-jv.com
Thu Dec 22 00:03:32 EST 2016


Hi Gunnar,

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!

I think all checks for daemon and daemon_local could be re-factored to use assert().
Both structures (are both really needed?) are for internal use and parameter to nearly all functions in daemon code,
The daemon would just not work if these pointers are NULL. 

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.   

All dlt_daemon_control_* are called when a DLT Client message is received. Depending on the message type, these functions are invoked
by dlt_daemon_client_process_control which itself is called by dlt_daemon_process_client_message when data is received on a Client connection.
Callstack would look like:

dlt_daemon_handle_event
	-> dlt_daemon_process_client_message
		-> dlt_daemon_client_process_control
			-> dlt_daemon_control_*
			
Inside dlt_daemon_process_client_message, data is read from corresponding connection, so it is ensured that data is available at this point.
Therefore, also the pointer to msg could be handled with an assert.

To answer your question, all code under src/daemon is internal to dlt daemon and not called by someone else. But this raises the question if you want to trust 
all parts of the daemon or just inside same source file. For this, it might be necessary to check which functions should be static and which functions can be called
from other parts of dlt-daemon.
To take the example above, dlt_daemon_client_control is non-satic, whereas all dlt_demon_control_* function could be changed to be static. And then we can trust
dlt_daemon_client_control, because its directly under our control (and in the same source file). This also corresponds to your error handling category 1.

One could anyhow think about to make daemon, daemon_local pointers global to avoid having them as parameter to each and every function. In many cases they are
just needed because another function which is called need access to either one or both of these pointers.

Best regards
Christoph


-----Original Message-----
From: genivi-diagnostic-log-and-trace [mailto:genivi-diagnostic-log-and-trace-bounces at mailman1.genivi.org] On Behalf Of Andersson, Gunnar
Sent: Wednesday, December 21, 2016 5:08 PM
To: Lipka, Christoph (ADITJ/SWG); genivi-diagnostic-log-and-trace at mailman1.genivi.org; Alexander.AW.Wenzel at bmw.de
Subject: Re: [genivi-dlt] [patch] check payload length before casting

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

_______________________________________________
genivi-diagnostic-log-and-trace mailing list genivi-diagnostic-log-and-trace at mailman1.genivi.org
http://lists.genivi.org/cgi-bin/mailman/listinfo/genivi-diagnostic-log-and-trace


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