TC6Regs_CheckTimers NULL pointer issue

[Andrewpini]

Hi, I'm currently working on integration of LAN8651.

During testing I've discovered that TC6Regs_CheckTimers does not seem to check if pReg->pTC6 is a NULL pointer.

How to reproduce:

• Avoid initializing TC6 registers through TC6Regs_Init().
• Call TC6Regs_CheckTimers()
• In my setup I ended up with a deadlock here:

  oa-tc6-lib/libtc6/src/tc6.c

  Line 237 in 8cc5240

  while(SPI_OP_INVALID != g->currentOp) {};

  (TC6Regs_CheckTimers->DoInitialization->TC6_Reset), caused by g->currentOp containing a garbage value.

The reason why this happens is that avoiding to call TC6Regs_Init() prevents GetContext from ever being called before DoInitialization(), causingpReg->pTC6=NULL, ref:

oa-tc6-lib/libtc6/src/tc6-regs.c

Line 214 in 8cc5240

pReg->pTC6 = pTC6;

Since TC6Regs_CheckTimers() must run periodically, and it will try to use pReg->pTC6 for several operations, it should check that it is a valid pointer.

As a last remark I want to emphasize that I have not done exhaustive testing/investigation of the internals here (yet), but I think it would be prudent to do a thorough investigation to check if/how this applies to other parts of the TC6 implementation.

Regards,

Anders Storrø

[tkummermehr]

Hi Andrew,

as mentioned in the tc6.h it is forbidden to call any function of the library until TC6_Init() has returned a pointer which is non NULL:

/** \brief Initializes the lwIP Interface Driver for TC6.

• \note Must be called before any other functions of this component.
  */

In my opinion its better to crash the application to detect an integration error (as you clearly did), as to silently ignore the error and continue. In that way, you are forced to fix the error in order to continue.

So will close this ticket.
Best regards,
Thorsten

[Andrewpini]

@tkummermehr

In my opinion its better to crash the application to detect an integration error (as you clearly did), as to silently ignore the error and continue. In that way, you are forced to fix the error in order to continue.

Well, I guess that depends on how we define crash. In my case I got stuck in an infinite loop, e.g. deadlock, which I'm not certain I would call a crash. To the best of my knowledge, accessing a NULL pointer does not guarantee that the application will crash. It will gurantee undefined behavior which of course can cause the application to crash, but also it can cause corruption of data, security vulnerabilities, etc.

I agree that you should not silently ignore any issues, but I do not get how guarding against reading/writing NULL pointers is in conflict with this goal. Could not the detection of a NULL pointer cause the TC6 regs procedure to abort and return an error, or as an alternative ASSERT? IMO this seems like a more graceful way to fail/crash.