drivers: modem: ublox-sara-r4: fix socket descriptor leak #28825
Conversation
drivers/modem/ublox-sara-r4.c
Outdated
| NULL, 0U, buf, | ||
| &mdata.sem_response, MDM_CMD_TIMEOUT); | ||
| if (ret < 0) { | ||
| LOG_ERR("%s ret:%d", log_strdup(buf), ret); |
There was a problem hiding this comment.
Perhaps we should make this a LOG_WARN now? I feel like users may see this error more often if we do an unconditional +USOCL.
There was a problem hiding this comment.
I'm okay with that change. One error condition being prevented here was issuing a +USOCL for a socket which was already closed by the remote peer. This wouldn't be unusual to encounter in a ported POSIX application so a lower log level sounds good to me.
We could also change the code to have something like an is_allocated or is_valid flag so we can catch that case but still allow sending +USOCL on an unconnected socket.
There was a problem hiding this comment.
I think that the log should not be displayed when we already now that the socket has been closed, and that we made the call only to avoid a leak in sockets in the modem internals.
A TCP socket is only closed with +USOCL if the socket is in the connected state. It is valid to call `close()` on a socket which is not connected, but in this case the underlying descriptor in the modem will not be released. This leads to a leak in sockets and eventual exhaustion. Issuing the +USOCL command unconditionally lets us avoid corner cases and allows for the module to check validity. Signed-off-by: Adam Porter <[email protected]>
|
@aport I'm not sure to understand, you mean that socket are leaking in the internal of the ublox sara modem (not its zephyr driver)? |
|
@XavierChapron The issue is the Zephyr driver preventing the transmission of +USOCL command when a TCP socket is not connected. In POSIX you can call In my latest round of testing, I can not reproduce the original bug where the SARA does not release a socket when +USOCO fails. I'll need to do some more testing. However as mentioned above, there's still a bug in the Zephyr driver that should be fixed. I will change the logic in this PR so that +USOCL is only sent to the modem if the socket has not been explicitly closed already. This will require a new flag separate from To summarize the issue:
If you repeat this seven times the modem will exhaust its internal socket pool and you will be unable to create new sockets. |
|
@aport Got it! Thanks for the explanation, I previously missed the part about calling close() without any connect() before it. |
|
I'm confused at the moment... I may close this PR until I have more time to sit down and look deeper into the issue. Reading the code, +USOCR is only called from |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
A TCP socket is only closed with +USOCL if the socket is in the
connected state. It is valid to call
close()on a socket which isnot connected, but in this case the underlying descriptor in the modem
will not be released. This leads to a leak in sockets and eventual
exhaustion.
Issuing the +USOCL command unconditionally lets us avoid corner cases
and allows for the module to check validity.
Addresses #26819
Signed-off-by: Adam Porter [email protected]