Dangling pointer may result in stack corruption

[henrygab]

Operating System

Others

Board

Any build using src/class/usbtmc/usbtmc_device.c class driver

Firmware

CodeQL reported:

Local variable address stored in non-local memory

While this alert is only a warning, further investigation suggests this
may be a latent stack corruption bug, and whether the corruption occurs
may be affected by compiler optimization settings (or prevented when
compiled with other optimization settings). Thus, this appears to be
a subtle bug that is extremely difficult to catch, and which may disappear
when using debug builds.

Therefore, please review carefully the description provided, which walks
the call tree with explanation.

What happened ?

Relevant call stack

In usbtmcd_control_xfer_cb(), while handling USB488_bREQUEST_READ_STATUS_BYTE:

• intMsg is declared as a local (stack) variable.
• &intMsg is passed as the buffer parameter to usbd_edpt_xfer()

Within usbd_edpt_xfer(), the buffer parameter is passed as the buffer parameter to dcd_edpt_xfer().

Requirements for the buffer parameter

The contract for dcd_edpt_xfer() is generally described in the porting document:

Once the transaction is going, the interrupt handler will notify TinyUSB of transfer completion.
During transmission, the IN data buffer is guaranteed to remain unchanged in memory until the
dcd_xfer_complete function is called.

Said another way, it is a requirement that the buffer parameter to dcd_edpt_xfer() must point
to memory that remains valid at least until the dcd_xfer_complete() function is called. Therefore,
because usbd_edpt_xfer() does not ensure dcd_xfer_complete() is called, the same requirement
also applies to the buffer parameter to usbd_edpt_xfer().

Dangling Pointer Bug

It appears that the call at usbtmc_device.c@898 is incorrect, as intMsg is a local (stack)
variable, and its stack space may be reclaimed as soon as the variable's scope (the curly braces)
is exited (aka, immediately after the call to usbd_edpt_xfer().

Therefore, because &intMsg is the buffer pointer, and there is no waiting for the transfer
to complete before exiting the scope of that local (stack) variable, the buffer pointer will
potentially still be in use when that stack location is reused for other purposes.

Not mitigated by hardware

It's possible that some specific hardware happens to ensure the transfer is synchronous,
which can prevent the effect of the bug from occurring.
Similarly, if the the asynchronous transfer happens to complete before the stack location
is re-used, the effect of the bug can remain hidden.

However, dcd_edpt_xfer() appears to be a hardware-specific routine, which makes reliance
on this being a synchronous request unwise. Moreover, each project using this source may
compile the code with different optimization settings ... making this a true Heisenbug.

How to reproduce ?

Review of the source, using the above description to follow the problem.

Debug Log as txt file (LOG/CFG_TUSB_DEBUG=2)

No debug log is required. This is a heisenbug, which disappears with timing changes / compilation optimization changes.

Screenshots

No response

I have checked existing issues, dicussion and documentation

• I confirm I have checked existing issues, dicussion and documentation.

[henrygab]

This is a heisenbug ... heavily dependent on timing / compilation options / etc.

This requires manual review of code to see the problem, so I worked multiple hours to provide a clear, concise explanation above, with links to code for quick reference.

Thank you for taking the time to review this bug report!

[henrygab]

UPDATE: For IN transfers (to host), this bug would instead mean that the data sent to the host can sometimes get corrupted.

Are there unwritten rules requiring that certain requests are completed synchronously? Is there some wait for dcd_xfer_complete() that occurs that I've missed in manual review?