SNTP fails to close the used socket

[johann2]

Describe the bug
Seems that the close function used in sntp_close will resolve to some close function not related to sockets instead of the one that calls zsock_close. As a result the socket remains open.

To Reproduce
I haven't been able to put together a minimal example to reproduce this.
The issue seems to happen only when CONFIG_NET_SOCKETS_POSIX_NAMES=n

Expected behaviour
sntp_close should close the socket

Impact
This is a blocking issue for us, because once enough sockets have been left open, no new sockets can be opened and the system loses connectivity.

Logs and console output
N/A

Environment (please complete the following information):

• OS: Linux
• Toolchain Zephyr SDK 0.11.3
• Commit SHA 0a7ea88

Additional context
It seems to me that using the zsock_* functions as proposed in #24129 would be the easiest fix and would bypass the problem completely. I'd be happy to bring this PR up to date if needed.

However, this bug may be a symptom of something else wrong in the POSIX api. I hope someone who's more familiar with this part of the codebase could take a look. #31323 seems to be somehow related to this bug as well.

[jukkar]

@pfalcon can you take a look at this as this seems to be related to the Posix API issues we have been seeing occacionally.

[pfalcon]

@johann2:

Seems that the close function used in sntp_close will resolve to some close function
I haven't been able to put together a minimal example to reproduce this.

That doesn't sounds too reassuring. To fix the issue, we need to see the issue. So, please try to provide steps to reproduce (ideally, reproduction code + build files).

The issue seems to happen only when CONFIG_NET_SOCKETS_POSIX_NAMES=n

Then the likely reason is that SNTP should depend on CONFIG_NET_SOCKETS_POSIX_NAMES=y (or POSIX_API=y). I can prepare such a patch.

Toolchain Zephyr SDK 0.11.3

The latest SDK release is 0.12.4. 0.11.3 is 4-5 releases old. Please upgrade and retry.

It seems to me that using the zsock_* functions as proposed in #24129 would be the easiest fix and would bypass the problem completely. I'd be happy to bring this PR up to date if needed.

As best as I remember, SNTP is a completely "userspace" lib, and so intended to work with native POSIX API, not adhoc Zephyr wrappers (for example, there could be counter-proposal to remove zsock_* prefixes, though I don't think anybody came up with that yet). So, I personally can't recommend such a workaround. (But maybe somebody else will.)

However, this bug may be a symptom of something else wrong in the POSIX api.

Yes, POSIX API is experimental and needs more work. For that, we need tangible examples of problem cases, and seek to find solutions, not workarounds.

[johann2]

An update:
The issue can be easily reproduced using the sntp_client example.
Set CONFIG_NET_SOCKETS_POSIX_NAMES=n and change the inet_pton function calls in the example to zsock_inet_pton
I've also updated the SDK, however it didn't affect anything.

As best as I remember, SNTP is a completely "userspace" lib, and so intended to work with native POSIX API, not adhoc Zephyr wrappers

I still don't understand why depending on POSIX api is desireable. It just makes tracking down things a lot harder. The API is not properly namespaced, which means it's prone to issues like these, and it pollutes the namespace everywhere sntp.h is included.

It could be argued that if all other functions are properly namespaced, this wouldn't be an issue, but unfortunately there's a lot of weird code out there, especially in the embedded world.

Is the zsock_* API unstable or an internal api?

[jukkar]

Is the zsock_* API unstable or an internal api?

No, it is a public API. The BSD socket API calls the zsock_ functions.

[pfalcon]

@johann2 : Good timing. I also was looking into this stuff and posted #34244.

Set CONFIG_NET_SOCKETS_POSIX_NAMES=n

So, the question is why do you set that? Per the current state of affairs (not how it was 5 or so years ago), if you use networking in an application (== sockets), either CONFIG_POSIX_API or CONFIG_NET_SOCKETS_POSIX_NAMES should be set. Or, you're doing something very peculiar. What?

I've also updated the SDK, however it didn't affect anything.

Thanks. It at least ensures that we're on the same line, and there's less search space to wander thru. (And some time ago there were changes in the toolchain regarding handling stuff like close()).

I still don't understand why depending on POSIX api is desireable.

Well, I don't think that phrase conveys the full picture of it. BSD Socket API is a part of POSIX API, so that's how it gets into picture. But we exactly don't want to force use of the full POSIX subsys just to use sockets, that's why we have CONFIG_NET_SOCKETS_POSIX_NAMES. Finally, when Sockets API was initially developed ~5 years ago as an experimental project, there was also a requirement to namespace it, mostly from concerns of integrating with vendor network offloading engines, which themselves sometimes provide socket-like API. This last usecase is the least (if at all) used, and shouldn't be of concern for most users. #34244 tries to address that by making sure that either CONFIG_POSIX_API or CONFIG_NET_SOCKETS_POSIX_NAMES is enabled, and only users which have peculiar needs take explicit action (disabling it).

The API is not properly namespaced, which means it's prone to issues like these, and it pollutes the namespace everywhere sntp.h is included.

Which API exactly is not properly namespaced? BSD Socket API is the cross-OS de jure and de facto standard for networking connectivity. In that regard its more reasonable expect that SNTP (network protocol) depends on standard cross-platform networking API, than on adhoc Zephyr-specific API.

It could be argued that if all other functions are properly namespaced, this wouldn't be an issue, but unfortunately there's a lot of weird code out there, especially in the embedded world.

Yes, that's why using zsock_ prefixes was among the requirements for the initial implementation. Fast forward a few years, and it seems more like overly conservative, reinsuring constraint, which causes more confusion and maintenance work than real boon.

Is the zsock_* API unstable or an internal api?

It's an implementation detail of Zephyr RTOS, otherwise being on par with the standard cross-platform API. That's why I'm trying to understand why people aren't using the standard API (and even propose to switch standard API to Zephyr-specific in existing code).

[johann2]

So, the question is why do you set that? Per the current state of affairs (not how it was 5 or so years ago), if you use networking in an application (== sockets), either CONFIG_POSIX_API or CONFIG_NET_SOCKETS_POSIX_NAMES should be set. Or, you're doing something very peculiar. What?

In our main codebase this option was implicitly set to n as I wasn't aware it's required for SNTP. I only set it to n to reproduce this using the sntp_client sample.

That's why I'm trying to understand why people aren't using the standard API (and even propose to switch standard API to Zephyr-specific in existing code).

The zsock_* API is in my opinion better. While not a global standard, zsock_close is more clear and less ambiguous than just close, especially for someone who hasn't done a lot of low-level unix C programming. Since the SNTP code is part of the zephyr code and isn't meant to be ported to other OSes, I don't see why public Zephyr API can't be used here, especially if it would be clearer and more readable, fix a bug and remove dependency on an experimental API.

In any case, I don't feel strongly enough about this question to push it further. There's a lot of ways to fix this bug and whatever you decide to do would work for me.