QUERY · ISSUE
[ESP32 port] network_ppp does not manage pcb properly; can cause LWIP assertion or unmanaged pcb
bug
In handling of active(false), network_ppp.c ignores the return value of pppapi_free() and always sets self->pcb to NULL. However, pppapi_free() can fail if the PPP link is not currently dead, in which case the PCB remains allocated.
The implication of this is:
- The PCB is now freed and yet network_ppp.c no longer has a reference to it. The PCB is essentially 'leaked' at this point.
- Subsequent attempts to call active(true) will cause an LWIP assertion (which leads to a panic) due to the fact that the network interface we are trying to add already exists (due to the previously-unfreed pcb).
- Also note that since the pcb didn't get freed, it's possible that the ppp_status_cb() function will be called by LWIP. If it does, the fact that self->pcb is set to NULL can cause an Access Violation (or Load Prohibited) since the ppp_status_cb() function assumes self->pcb is non-NULL.
The scenario above is exacerbated by the fact that:
- network_ppp.c has a 4000 ms timeout for the pppapi_close() call. However, if pppapi_close() was called while the LCP finite state machine had not yet set the phase of the PPP to DEAD, then LCP layer will try to send 2 TERM_REQs to its peer.
- If the peer is not answering, both of these TERM_REQs will timeout. In the version of LWIP, the timeout is configured for 6 seconds per attempt, for a total of 12 seconds. Therefore the LCP layer will require more than the 4 (or 4000ms) given by network_ppp.c.
- As a result, active(false) will timeout on pppapi_close(). Furthermore its call to pppapi_free() will fail and this failure will trigger the initial problem described above where network_ppp.c has NULLed its reference to the PCB eventhough the PCB hasn't been freed successfully in LWIP.
CANDIDATE · PULL REQUEST
network_ppp: Stop polling if stream or PPP PCB are removed.
port-esp32extmod
Summary
Two small fixes that improve when the poll() method stops looping trying to read more data from the stream:
- When the stream is set to
None.- This was already intended and the case in the ESP32 version when I initially implemented support for the setting
streamto None, but I didn't correctly port it over to the extmod version. And then later I brought the bug from the extmod version back to the ESP32 version. 😞
- This was already intended and the case in the ESP32 version when I initially implemented support for the setting
- When PPP is disconnected.
- When disconnecting from PPP the modem sends a confirmation. This message is received, like all messages, through the
poll()method. lwIP may then immediately call our status callback with codePPPERR_USERto indicate the connection was closed. Our callback then immediately proceeds to free the PCB. Thus, during each new iteration of the loop inpoll()we must check if we haven't disconnected in the meantime to prevent calling thepppos_input_tcpipwith a PCB that is now NULL.
- When disconnecting from PPP the modem sends a confirmation. This message is received, like all messages, through the
Testing
Tested on the ESP32 by disconnecting and observing we no longer crash due to self->pcb being NULL.
Updated the extmod version to keep the two versions in sync. Haven't tested it on a port using the extmod version, but it looks like a correct condition to check there as well.