On Wed, 20 Sep 2017, Denis Kenzior wrote:
On 09/20/2017 06:40 PM, Mat Martineau wrote:
> I ran ELL through a static analysis tool and came up with a lot of hits.
> Only a handful were bogus, and there are also some that definitely require
> fixes (I'll send patches).
> There are a few broad classes of issues that I wanted to get some feedback
> on before making changes.
> 1. Ignored returned error values from l_io_get_fd()
> l_io_get_fd() returns -1 when it's passed a NULL l_io pointer. In many
> places, that -1 gets passed to a system call as a file descriptor. Those
> system calls typically return an error when they are given a bad fd, and
> there is typically proper error handling for the system call.
> Many of these cases involve passing in a pointer that's only used by ELL
> internals and is never set to NULL anyway. What do you think about leaving
> these alone as long as there's a defined error path (even if that error
> path involves a system call that could have been skipped)?
I bet almost all of these are inside read/write handlers. read/write
handlers will not be called on a NULL io object or if the underlying fd
becomes invalid. So you're guaranteed that the io object is valid. These can
be left alone.
I checked a few more examples - they are mostly read/write handlers.
> 2. Ignored returned error values from l_hashmap_insert()
> l_hashmap_insert() returns false when passed a NULL l_hashmap pointer. This
> isn't always checked, especially when the l_hashmap is something used by
> the ELL internals and is never set to NULL.
> Seems like these are low risk if it can be confirmed that the pointers
> aren't set to NULL during their owner's lifetime.
We can look at these, but generally the hashmaps we use are created at object
allocation or first insertion and destroyed when the object is destroyed. So
likely most of these can be ignored.
> 3. strcpy() instances that could easily be strncpy()
> A number of calls to strcpy() were flagged that are copying to destinations
> of known length.
> These should get fixed, especially where the source string comes from
So the particular case you bring up is actually a good example of why we
_don't_ want to convert every single strcpy into a strncpy.
1. The signature is validated by the logic above to be no more than 255
characters (e.g. _gvariant_valid_signature).
2. The builder object is meant to be used by the programmer to build D-Bus
messages. It isn't meant to receive arbitrary input from the network, etc.
So in this case, even if somehow a buffer overflow occurs, I want valgrind to
warn the programmer ASAP and tell them exactly where they screwed up instead
of having a silent and hard-to-find failure.
Our principle of crash-early applies...
Crash early, but only when running valgrind/asan/etc? When it's trivial
to check for forseeable misuse, especially with regard to string overruns,
we can deterministically crash (or otherwise fail) early.
> non-ELL code. I can't think of a reason to use strcpy() over strncpy()
> unless the source string is hard-coded, and even then strncpy() isn't a
Ok, one reason: strncpy() has the additional cost of zeroing the unused
space at the end of the dest buffer, so if the src length is otherwise
checked strcpy() doesn't hurt.
strncpy is a pain in the ass due to null termination semantics and strcpy is
such an obvious attack vector that any code introduced with it gets
Static analysis helps catch strncpy misuse - it caught some off-by-one
strncpy parameters that we need to fix.
I briefly grepped and looked at our use of strcpy and everything I found was
completely and obviously okay. Lets have a deeper look, but I'm totally not
agreeing with your assertion above ;)
Agree with the deeper look. I've also revised my assertation :)
> 4. Ignored errors in test/example code
> These are low risk because they aren't in the library itself. However,
> tests and examples are used as templates for new programs. Does it make
> sense to make the examples (and maybe the tests) squeaky-clean in terms of
> error handling?
Yeah I'd vote to have that one fixed since the rest of the example provides
proper error handling.
> To take a step back, it's one thing to fix these bugs now, but another to
> have something in place to catch future problems. I think it would be good
> to set up ELL for scanning on an opensource-friendly static analysis site -
> I'll volunteer to babysit that process too.
That would be nice!
I got this mostly set up today, and am waiting for approval. I'll post
information to the list once the project is approved and accessible.