Define _POSIX_C_SOURCE in a few files which use strnlen()

[aescolar]

Define _POSIX_C_SOURCE in a few files which use strnlen()

As newlib does not expose by default strnlen(), we need to set this macro, otherwise we will get an
"implicit declaration" warning when building for newlib or other C libraries which do not expose it.

These cases were missed from #67052 as they were not triggered by CI as we do not build by default with newlibc.

[sylvioalves]

Thanks. @aescolar @cfriedt @keith-packard, I suggested this removal during POSIX discussions, but as you see, newlib requires it and I missed it. Is there any better solution for this scenario besides forcing this in the source file?

[aescolar]

Thanks. @aescolar @cfriedt @keith-packard, I suggested this removal during POSIX discussions, but as you see, newlib requires it and I missed it. Is there any better solution for this scenario besides forcing this in the source file?

There is an ongoing discussion about having Newlib set it, #67040 (comment) , or change its integration so it exposes this funcion by default but that has consecuences, so it won't happen right away.
The macro can also be set for just this file in cmake.

[kartben]

How about https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/wifi/eswifi/eswifi_core.c#L493 (not that this file was touched in the reverted commit, but I'd guess it's impacted too?)

[kartben]

And actually, how about the handful of other files in tree that use strnlen and don't have the macro set? ex. subsys/llext/shell.c, or subsys/ipc/ipc_service/backends/ipc_icbmsg.c

[aescolar]

And actually, how about the handful of other files in tree that use strnlen and don't have the macro set? ex. subsys/llext/shell.c, or subsys/ipc/ipc_service/backends/ipc_icbmsg.c

@kartben I guess they were not normally being built before with newlib, and are not built either now. So nobody has realized or realizes yet about it. We default to picolibc, so this issues will be quite hidden. I only realized about this particular case because of your report in the other PR.

[cfriedt]

Personally, I think we should be using the ISO C standard variant of strnlen() (namely strnlen_s()) where POSIX is not intended to be used. It's likely available in all libcs except maybe the minimal one, but adding it there is a trivial change.

https://en.cppreference.com/w/c/string/byte/strlen

strnlen_s() is a drop-in replacement for strnlen().

There would be one less special case to consider in terms of coding guidelines, as a bonus.

[aescolar]

Personally, I think we should be using the ISO C standard variant of strnlen() (namely strnlen_s()) where POSIX is not intended to be used.

strnlen_s() apart from not being in the list of allowed functions is part of the Annex K “Bounds-checking interfaces”, which is explicitly not allowed by our coding guidelines.
So any fix in that line would require a much longer discussion, update of the coding guidelines and change of all uses in-tree.
You are welcome to start an RFC to propose that overall strnlen be replace with strnlen_s if you desire.
But given that this issue is currently present using a function which is currently allowed by the project coding guidelines, and that such a proposal may not lead to a conclusion, I would think it best to fix it how it is done in this PR.

---

strnlen_s() does not seem to be avaliable in pico, new or glibc.

[cfriedt]

strnlen_s() apart from not being in the list of allowed functions is part of the Annex K “Bounds-checking interfaces”, which is explicitly not allowed by our coding guidelines.

I was looking for this section in the coding guidelines but had some difficulty finding it

So any fix in that line would require a much longer discussion, update of the coding guidelines and change of all uses in-tree.
You are welcome to start an RFC to propose that overall strnlen be replace with strnlen_s if you desire.

In that case, I guess it doesn't make sense to hold off for a potentially lengthy RFC process.

However, I do think that we should prefer sticking to ISO C rather than forcing POSIX APIs to be exposed in various places where it should not be (according to A.4 and A.5).

[cfriedt]

Not going to hold this up, but I do think we should use the ISO C variant (strnlen_s()) instead of forcing the entire POSIX API to be exposed when it isn't necessary or desired.

[keith-packard]

This change seems to move in the direction of not asking that the C library express the full Zephyr API by default, and effectively requiring any file using POSIX apis to add the _POSIX_C_SOURCE define. If this is the plan, then reverting the picolibc changes which expose the Zephyr API without additional defines, and changing the minimal C library to gate the POSIX APIS on _POSIX_C_SOURCE seems like a required additional change.

[kartben]

I'm curious as to why Annex K items are disallowed.

I could find this #57598 (comment)

[keith-packard]

This change seems to move in the direction of not asking that the C library express the full Zephyr API by default, and effectively requiring any file using POSIX apis to add the _POSIX_C_SOURCE define. If this is the plan,

I don't think it's the overall plan.

I'm confused then -- either the default Zephyr C Library API includes strnlen, or applications are required to set _POSIX_C_SOURCE to gain access to it. If we are changing code to add _POSIX_C_SOURCE defines, then I don't see how this could ever be taken as anything other than the latter choice. If this is just a temporary kludge to work-around the lack of Zephyr API support in newlib, then I think it would be far better to place this in the Zephyr build system (as proposed in #67040) rather than littering the tree with random instances as we happen to build bits with newlib.

Strnlen is probably a special case because it's one of the non-ISO-C APIs that Zephyr has chosen to support.

Exactly. We have two choices here:

1. Define a Zephyr C Library API that includes ISO C + a few additional functions. Files using only this API would need no additional _*_SOURCE defines.
2. Use the POSIX C Library API, with caveats that the Zephyr implementation is incomplete. Source files using APIs defined only in the POSIX spec would need to define _POSIX_C_SOURCE to suit.

I had thought the directly from the TSC was clear -- Zephyr defines its own C library API and applications that limit themselves to that API need not define any _*_SOURCE macros to use it. With the addition of these _POSIX_C_SOURCE defines, I appear to have been mistaken, and that Zephyr has adopted choice 2. instead. That's all fine with me; I'll go revert the Zephyr-specific API changes in Picolibc as they are not correct, and also harmful as they incorrectly assert a C library API broader than the ISO-C library Zephyr assumes.

We can easily support strlen_s with inside of lib/libc/common (and strlnlen inside of lib/posix)

Not in any standards-conforming fashion though; Annex K doesn't allow for subsetting, so the library would need to set __STDC_LIB_EXT1__ and define all of Annex K. Given the paucity of Annex-K conforming implementations, the broad availability of POSIX-conforming implementations that include strnlen, it's easy to see why developers choose the POSIX name rather than the ill-fated Annex-K name.

I'm curious as to why Annex K items are disallowed.

Probably because few (if any) C library implementations have adopted this part of the spec. It's a pretty big lift as well; a huge part of the library ends up with two parallel implementations. https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1967.htm has a pretty good explanation of how Annex K has failed to see any significant adoption. In the case of strnlen, it would have been far better for the ISO C committee to simply include that function in the spec, rather than creating a new name for the POSIX function with, what appears to be, identical semantics. Essentially, if you want to provide strnlen_s, you have to buy-in to the whole of Annex K.

[cfriedt]

This change seems to move in the direction of not asking that the C library express the full Zephyr API by default, and effectively requiring any file using POSIX apis to add the _POSIX_C_SOURCE define. If this is the plan,

I don't think it's the overall plan, at least, not according to the coding guidelines.

Strnlen is probably a special case because it's one of the non-ISO-C / POSIX APIs that Zephyr has chosen to support.

We can easily support strlen_s() (no guard necessary) and strnlen() (guarded by _ZEPHYR_SOURCE) inside of lib/libc/common and it would be a very simple change. In fact there is already a PR to do that partially.

The following paragraph (from rule A.5 seems to articulate the same point I was trying to make earlier.

Adding a new non-standard function from common C standard libraries to the above list is allowed with justification, given that the above requirement is satisfied. However, when there exists a standard function that is functionally equivalent, the standard function shall be used.

Maybe @stephanosio could shed some light.

Personally, I think using the standard strlen_s() instead of the non-standard strnlen() seems to be fully in line with the coding guidelines.

There might be some unintentional overuse of the term "standard" in the quoted paragraph above (maybe it should read "common C libraries"?).

[keith-packard]

Personally, I think using the standard strlen_s() instead of the non-standard strnlen() seems to be fully in line with the coding guidelines.

strnlen is certainly a standard interface, it's just defined by the POSIX standard, not the ISO C standard. And, if you want strnlen_s, then the only way to get it that is defined in the ISO C standard is to provide all of Annex K. Essentially any road we pick here is not aligned with any existing standard.

In this particular case, I'd argue pretty strenuously that using strnlen is the correct choice -- it's far more familiar to existing C developers than strnlen_s and is already provided in many common C libraries. Anyone with experience of Annex K will look at all *_s functions with concern -- they're fraught with potential peril of what happens when "runtime constraints" are violated, and have occasionally surprising semantics (checkout how strncpy_s compares with strncpy).

There may be some unintentional overuse of the term "standard" in the quoted paragraph above (maybe it should read "common C libraries"?).

No, I think "standard" is the right term. Any API which is not part of a commonly implemented de jure standard should be looked at with a wary eye.

We're looking for well-defined and well-supported interfaces here. POSIX is definitely a good source for interfaces of this kind; managed by a respected standards body (IEEE) and broadly implemented across numerous systems (Linux, BSD, Windows). Annex K fails this test as it has not only no usable implementations (even the Windows C library is non-conforming), it has been intentionally excluded from several (including Glibc).

[cfriedt]

Personally, I think using the standard strlen_s() instead of the non-standard strnlen() seems to be fully in line with the coding guidelines.

strnlen is certainly a standard interface, it's just defined by the POSIX standard, not the ISO C standard.

Correct. And I am well aware of that 😁. I was using the definition of "non-standard" per the coding guidelines (below).

The following non-ISO 9899:2011, hereinafter referred to as non-standard, functions and macros are exempt from this rule and allowed to be used in the Zephyr codebase:

In any case, we can also move strnlen to lib/libc/common or lib/posix, and make that function visible when either _POSOX_C_SOURCE or _ZEPHYR_SOURCE is defined.

In this particular case, I'd argue pretty strenuously that using strnlen is the correct choice

Cool - let's put it in lib/lib/common and guard the declaration with _POSIX_C_SOURCE or _ZEPHYR_SOURCE.

Problem solved 😃 No unnecessary definitions of _POSIX_C_SOURCE required.

In fact, we don't even need to move the implementation, all we need to do is declare it.

[cfriedt]

Temporary nak until a better solution is available.

[keith-packard]

In fact, we don't even need to move the implementation, all we need to do is declare it.

Yeah, we don't want to replace any C library implementation with something from libc/common -- the C library may well have a nice optimized version (as newlib and picolibc do for aarch64).

However, it needs to be declared in <string.h>, which is provided by the C library. Do you propose creating a Zephyr version of string.h that uses #include_next <string.h> to pull in the C library version? Will use of that be limited to C libraries that don't have Zephyr support and which will already declare the Zephyr API additions?

Oh. Maybe this kludge should go into the newlib support bits -- have that provide a string.h which dtrt for newlib. Other C library integration bits could steal the same kludges or fix their upstream to add Zephyr support. Or stick the kludge into common somewhere and have the newlib code enable it; that would enable sharing the kludge more easily.

(the same should go for strtok_r() too, right?)

[cfriedt]

Please see #68261 for a simpler alternative.

@aescolar - do you know of any specific build failures? Is there a bug filed for this issue? Are there any specific tests I should try to reproduce?

[cfriedt]

(the same should go for strtok_r() too, right?)

@keith-packard - you are a mind reader! 😅

[aescolar]

This change seems to move in the direction of not asking that the C library express the full Zephyr API by default..

@keith-packard , @cfriedt , @kartben
I have created an RFC (#68278) to discuss this matter, as it is otherwise quite difficult to follow and document a discussion spread between several PRs and issues. Let's move this discussion there.

As a short answer, this PR just tries to ensure this files build well with newlib (or other libCs) without the issue of defining globally _POSIX_C_SOURCE or trying to resolve the topic discussed in #68278

[kartben]

As a short answer, this PR just tries to ensure this files build well with newlib (or other libCs) without the issue of defining globally _POSIX_C_SOURCE or trying to resolve the topic discussed in #68278

Ya, and to, I think, an earlier point of yours, I think this should go in as a (hot)fix for now, since it can break for users, until better/other solutions are explored.

[aescolar]

Ya, and to, I think, an earlier point of yours, I think this should go in as a (hot)fix for now, since it can break for users, until better/other solutions are explored.

@cfriedt please consider this point from @kartben

[cfriedt]

Ya, and to, I think, an earlier point of yours, I think this should go in as a (hot)fix for now, since it can break for users, until better/other solutions are explored.

@cfriedt please consider this point from @kartben

I did, originally, until it was mentioned that this PR should set some sort of precedent making blanket use if _POSIX_C_SOURCE OK, which I think is definitely not any kind of conclusion that should be extrapolated from this.

Unfortunately, I would have to say that #68261 is still technically a better solution and there have not been any compelling arguments otherwise.

[keith-packard]

I've rewritten #67040 so that newlib now provides the Zephyr API, including strnlen and strtok_r, even when _POSIX_C_SOURCE is not defined. That should avoid the need for these defines unless we want to support other C libraries and cannot use a similar wrapper.

[stephanosio]

We can easily support strlen_s() (no guard necessary) and strnlen() (guarded by _ZEPHYR_SOURCE) inside of lib/libc/common and it would be a very simple change. In fact there is already a PR to do that partially.

The following paragraph (from rule A.5 seems to articulate the same point I was trying to make earlier.

Adding a new non-standard function from common C standard libraries to the above list is allowed with justification, given that the above requirement is satisfied. However, when there exists a standard function that is functionally equivalent, the standard function shall be used.

Maybe @stephanosio could shed some light.

Personally, I think using the standard strlen_s() instead of the non-standard strnlen() seems to be fully in line with the coding guidelines.

While strlen_s is technically part of the ISO C standard (optional Annex K), it is very controversial, (arguably) flawed, and despised by the GNU people (though, this might actually have more to do with Microsoft being an advocate of the Annex K).

All that aside, the adoption rate of the strlen_s alongside other functions from the Annex K is close to zero and even the WG14 themselves are questioning its future.

From N2336:

Annex K of C11, Bounds-checking interfaces, introduced a set of new, optional functions into the standard C library with the goal of mitigating the security implications of a subset of buffer overflows in existing code. The bounds-checked interfaces originated as an ISO/IEC technical report in 2007 before being incorporated in C11 as the optional but normative Annex K. Field experience with the bounds-checked interfaces has been hampered by a lack of adoption by compiler implementations, despite these interfaces being available for over a decade. This lack of adoption resulted largely from unfounded criticism of the API as well as actual flaws. As a result, the international standardization working group for the C programming language is evenly divided between repairing and eliminating Annex K for the next major release of the C language (C2X).

Based on the above, I would argue that, for all practical purposes, strnlen is more (de facto) standard than strlen_s and Zephyr stick with it. As previously discussed in the Security WG, Zephyr should refrain from adopting the optional Annex K because it is too controversial.

[cfriedt]

Based on the above, I would argue that, for all practical purposes, strnlen is more (de facto) standard than strlen_s and Zephyr stick with it. As previously discussed in the Security WG, Zephyr should refrain from adopting the optional Annex K because it is too controversial.

Thanks for clarifying @stephanosio .

Keith's solution in #68381 works for me in terms of newlib

[aescolar]

Replaying to #68233 (comment) , @keith-packard

[..] either the default Zephyr C Library API includes strnlen, or applications are required to set _POSIX_C_SOURCE to gain access to it. [..]

Note that there is no such dichotomy. There is other C libraries beyond the default C library (Pico) or beyond other libraries for which Zephyr may provide extra support for.
So, there is no contradiction in both having picolibc provide those prototypes by default, and setting this macro where it would be necessary for other C libraries.

[github-actions]

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.