Esp-idf: Instrumentation options (-fsanitize) & quality control (IDF-166)

[PerMalmberg]

Hi,

I'd like to raise the question on the feasibility of introducing support for the -fsanitize flags when targeting the Xtensa CPU. As the code base of ESP-IDF continues to grow, the risk of introducing bugs increases. This is of course not specific to ESP-IDF, but rather a common trait of any c-program/framework. However, we're now in 2018 and the tools we use daily (i.e. gcc) can actually support us in keeping our pointers and indexes in check and even tell us when we attempt to dereference a null-pointer and and other bad things. I'm not a compiler developer so I don't know if it even is possible to support this for the Xtensa architecture, but it'd be worth so much.

Naturally, such instrumentation comes at a cost in memory usage etc., but I'm sure many of us would be willing to pay that price during development to be sure that code in both ESP-IDF and our own project are free of these errors. Personally I'd be willing to pay the price of not being able to run all parts of my application simultaneously if I could turn on instrumentation.

The reason for me asking is that ESP-IDF is currently a large unknown to me in terms of actual code quality. I write all my code for ESP32 in C++, using the standard-library wherever possible to avoid raw pointers, raw arrays etc. On top of that I write tests for my code and compile those on Linux using _-fsanitse_ and _Valgrind_ to make sure I don't introduce any memory overwrites etc. (except for H/W drivers which only run on the ESP32).

ESP-IDF does provide some means to diagnose memory corruption, but being preventive is a much better option. I'm happy to see that there are unit tests for some components. Unfortunately, the only guarantee they give is that the tiny little application they make up functions; they give no guarantee that the code they test behaves well in a larger system, i.e. bounds checking etc.

Is Espressif running static/runtime code analysis and other tests internally that we as users can't see? If that is the case, please let us know. If not, how about doing that? I know it requires an effort, but it is well worth it as it gives us all confidence in the code we all build our applications on.

[igrr]

Hi @PerMalmberg,
static analysis support is tracked in https://github.com/espressif/esp-idf/issues/145. Currently we don't have a good solution which we could distribute (or allow others to use as a service), but this is a tools licensing issue rather than a technical issue. We can use static analysis internally, although it is not integrated into our CI / code review flows yet.

Regarding -fsanitise, i think @jcmvbkbc has been working on this in https://github.com/jcmvbkbc/gcc-xtensa/commits/xtensa-asan-7.2.0, but it seems that runtime libsanitiser support is only being added for Linux.

[projectgus]

Regarding code quality in genral, to add to what @igrr says,, as you've noticed we run a large suite of unit tests on every merge (including on the master branch before it's pushed from our internal server to github). You can run these tests yourself: http://esp-idf.readthedocs.io/en/latest/api-guides/unit-tests.html

We also run a suite of integration tests including end-to-end WiFi, Bluetooth, etc operations as part of the same process (ie before the master branch on Github updates). These run with multiple software configurations. These are not publically documented as they rely on a lot of fiddly internal infrastructure to run.

As Ivan says, though, we'd love to add static analysis to this mix if/when we can. And we're looking with interest at any ASAN approaches on "small" hardware like ESP32.

You can also enable gcc Stack Smashing Protection under compiler options now.

Angus

[jcmvbkbc]

Regarding -fsanitise, -fsanitize=undefined, aka UBSAN should be straightforward to use. Various __ubsan_handle_* callbacks may be implemented in the user code as they appear.
Regarding ASAN, I'll try to sketch an implementation for esp32. Compiler support is available in the gcc-7.3.

[PerMalmberg]

@igrr @projectgus Thanks for listening 👍

As long as code analysis is run on ESP-IDF I think we'll all be happy, even if that is accessible only internally at Espressif due to licensing. Personally, I think that such an added level of quality check would greatly increase the value that is provided with E-I, especially if you visualize the result of the effort made in this area.

That you are running the tests and more as part of the process to update the master branch make me happy to hear. Perhaps a few lines in the documentation about this is a good idea, letting us users know that there is in fact a process in place to prevent regressions (even if it isn't complete yet and things like #1443 may happen)?

@jcmvbkbc
Thanks for letting us know about UBSAN, I'll definitely look into implementing them. If ASAN can be added for esp32, that would be extremely valuable.

[igrr]

even if it isn't complete yet and things like #1443 may happen

We are in the process of setting up test environments for peripheral drivers test cases, and there are some MRs in progress which add these tests. It's a bit fiddly to set up/maintain these test environments, which is the main reason why we didn't have such test cases yet.

[PerMalmberg]

It's a bit fiddly to set up/maintain these test environments,

Agreed. I am well aware of the effort associated with testing software that runs only on specific hardware. You can only do so much at a time, and you're already doing great things 🥇 Pass on some of that praise to your colleagues too, you're all worth it.

[mcilloni]

Any news about sanitizers? We're tackling a very weird bug that clearly arises from undefined behaviour, but trying to build with -fsanitize=undefined results in sleight of linker errors due to libubsan not being available in Espressif's GCC builds.

[igrr]

@mcilloni UBSAN support is in review at the moment and will be available in the master branch in a couple of weeks. This issue will be updated when that happens.

[mcilloni]

@mcilloni UBSAN support is in review at the moment and will be available in the master branch in a couple of weeks. This issue will be updated when that happens.

Thank you very much for your super fast reply. I'll follow this issue for updates.