casaroli opened a new pull request, #21217: URL: https://github.com/apache/nuttx/pull/21217
## Summary POSIX declares `int putenv(char *string)`: the string itself becomes part of the environment, so a later change to the string changes the environment. NuttX declared `putenv(FAR const char *string)` and copied the string. A program that declares `putenv()` itself with the POSIX prototype failed to compile (conflicting types), and a program that changes the string after `putenv()` did not change the environment. - **libs/libc: Put the string of putenv() into the environment, as POSIX does.** `<stdlib.h>`, `env_putenv.c` and `libc.csv` use `char *`. `putenv()` puts the string itself into the environment array of libc: in the place of the old `name=value` pair, or at the end. libc does not copy or free it (`env_removevar()` and `env_release()` free only the blocks of `env_alloc()`). `NAME=` now sets `NAME` to an empty value, as in POSIX (before: it removed `NAME`). `env_setvar()` existed only so that `putenv()` could give the name part of its string to `setenv()`; it is moved back into `setenv()`. - **Documentation: Say that putenv() does not copy the string.** `Documentation/reference/user/09_env_vars.rst`. Merge order: apache/nuttx-apps#3948 first, then this PR. The apps PR works with the current NuttX `putenv()` too. Without it, ostest does not build with this PR: it passes a `const` array to `putenv()` (`-Werror=discarded-qualifiers`, also in the arm64 builds without `EXTRAFLAGS`). Also, the bas `ENVIRON` statement frees the string after `putenv()`. depends-on: apache/nuttx-apps/pull/3948 ## Impact All build modes (flat, protected, kernel). Programs see two changes: - Source: a `const char *` argument now gives a "discards const qualifier" warning (an error with `-Werror`); in C++ a string literal argument gives an error. A string literal is still fine in C. In-tree, nuttx has no caller; in nuttx-apps, ostest (const array) is fixed by the apps PR; `games/NXDoom` (a heap string and a static array, both kept for the whole program) and `examples/tlpi` (string literals, C) need no change. - Behaviour: the string must stay valid while it is in the environment, as with other POSIX C libraries. A program that gives a buffer that it frees or reuses after `putenv()` now leaves a stale entry. In-tree, only `interpreters/bas` did that; the apps PR gives it a copy. Breaking change: no. This is a POSIX conformance fix with a small API change, so the PR has no `!` and no vote. But the API also changes in the flat build: the prototype in `<stdlib.h>`, and the lifetime rule for the string. ## Testing Test program `putenvposix.c` (the string is shared, not copied; replace, `setenv()`/`unsetenv()` of the same name; no `=`; `NAME=`; 100 variables; `environ` assigned by the program; a `fork()` child) on `qemu-armv8a:knsh` (kernel) and as a builtin app on `qemu-armv8a:nsh` (flat), QEMU with HVF. The macOS libc passes all cases. Base: master at 23f5a4f06df (7505b874722, #21072, does not build for arm64; fixed by #21210). | nuttx + apps | putenvposix knsh (10 cases) | putenvposix flat nsh (9 cases) | ostest knsh / nsh / flat nsh | |---|---|---|---| | master + apps master | 7 FAIL | 7 FAIL | 0 / 0 / 0 | | master + apps PR | 7 FAIL | 7 FAIL | 0 / 0 / 0 | | this PR + apps PR | 0 FAIL | 0 FAIL | 0 / 0 / 0 | | this PR + apps master | ostest does not build (`ostest_main.c:817`, discards `const`) | same | - | On master, these cases fail: `proto` (`const char *`), `shared`, `replace`, `unsetenv` (the string stays in the environment), `empty` (`NAME=` removes `NAME`), `grow` and `environ` (the entries are copies). `setenv`, `noeq` and `fork` pass on master too. Builds with the CI flags (`EXTRAFLAGS="-Wno-cpp -Werror"`; the arm64 builds always have `-Werror`), this PR + apps PR: `qemu-armv8a:knsh`, `qemu-armv8a:nsh` (ostest), `rv-virt:pnsh64`, `rv-virt:knsh64`, `mps2-an500:knsh` (ostest): all OK. `qemu-armv8a:nsh` with ostest and `CONFIG_INTERPRETERS_BAS`: OK with `-Wno-error=array-bounds`. Without it, GCC 14.2 reports `-Werror=array-bounds` in `bas_statement.c:748-750`, on master + apps master too. `tools/checkpatch.sh -c -u -m -g` passes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
