pacemaker-remoted load_env_var refactors, and assorted minor changes #3833

nrwahl2 · 2025-03-03T09:52:00Z

I'm trying to fix the Coverity issues that are popping up for #3832 and these are also appearing on my machine. There are some more I haven't reached yet.

daemons/execd/remoted_pidone.c

nrwahl2 · 2025-03-03T19:19:22Z

Used a define per review (https://github.com/ClusterLabs/pacemaker/compare/17bb059c39e3ee0b75a8fc26c259a26c5241c719..3ebf24e3100e21a2900009ebcb5c5c7edafa92a1)

clumens · 2025-03-03T19:47:45Z

Were you able to test the changes to load_env_vars? That, or some unit tests, would make me feel a whole lot better about all that small batch string processing code.

nrwahl2 · 2025-03-03T20:33:38Z

Were you able to test the changes to load_env_vars? That, or some unit tests, would make me feel a whole lot better about all that small batch string processing code.

Not really. That's gonna take me a little while to set up and figure out (won't be ready today during business hours). But it is a good idea.

Also interested in whether there's a better way than rolling our own parser, but haven't found anything yet.

nrwahl2 · 2025-03-05T07:25:02Z

Not really. That's gonna take me a little while to set up and figure out (won't be ready today during business hours). But it is a good idea.

Also interested in whether there's a better way than rolling our own parser, but haven't found anything yet.

I'm planning to use pcmk__scan_nvpair() here (now that I'm aware it exists) and then process the key and value separately. It will hopefully make things easier to reason about, even though there will still be corner cases involving escapes and nested quotes for now. See also #3834.

I think the authors made the load_env_vars() function way too complicated in our habitual obsession with "try to never allocate heap memory for a string." Which makes sense in the kernel, but IMO it's pretty silly in MOST of Pacemaker, where we're allocating large data structures frequently. Way too complicated without even achieving totally correct behavior, because it's so easy to get bogged down in the character-by-character parsing details.

clumens · 2025-03-05T17:54:31Z

I'm planning to use pcmk__scan_nvpair() here (now that I'm aware it exists) and then process the key and value separately. It will hopefully make things easier to reason about, even though there will still be corner cases involving escapes and nested quotes for now. See also #3834.

Okay, I'll wait to do final review on this until after you've moved to using pcmk__scan_nvpair().

I think the authors made the load_env_vars() function way too complicated in our habitual obsession with "try to never allocate heap memory for a string." Which makes sense in the kernel, but IMO it's pretty silly in MOST of Pacemaker, where we're allocating large data structures frequently. Way too complicated without even achieving totally correct behavior, because it's so easy to get bogged down in the character-by-character parsing details.

I absolutely agree. If I were writing this function from scratch, I would (1) have it take file contents as a parameter instead of a file name, (2) put it in a library somewhere as private API to allow us to unit test it, and (3) find something in glib that could be used to do the parsing instead of writing it by hand. Now we're stuck with having to support whatever bugs there might be in processing since otherwise we'll break people's remote nodes.

nrwahl2 · 2025-03-06T07:30:45Z

Now we're stuck with having to support whatever bugs there might be in processing since otherwise we'll break people's remote nodes.

Not necessarily. Obviously we've run into many situations where users are relying on behavior they shouldn't be... but these corner cases seem like pretty clear bugs that shouldn't be guaranteed to work, and that it's unlikely anyone is using. I'd be willing to gamble, if we actually feel like fixing them.

That's not something I want to spend time on right now though. If we find a good parser in a library, fine. But setting flags for quote nesting levels, whether we just saw a backslash and are escaping the next character, etc., sounds really unappealing.

nrwahl2 · 2025-03-06T10:39:20Z

Rebased on current main, which led to dropping the final commit. Otherwise no changes.

daemons/execd/remoted_schemas.c

nrwahl2 · 2025-03-08T01:02:12Z

I added a commit to fix the rand() suppression. I also split up the load_env_vars() stuff into several more incremental commits and then used pcmk__scan_nvpair() in the final one. It doesn't really make that function any shorter, but IMO it's a lot more readable now.

I have not tested yet.

nrwahl2 · 2025-03-08T07:53:04Z

Still need to test the load_env_vars() stuff and maybe functionize the value validation part, and run cts-lab for a bit to maybe find any obvious issues with the remote message stuff.

I can't easily test the localized_remote_header() involving byte swapping for endianness, because I don't have a big-endian machine. The last time I tried to set up a big-endian VM, if I ever succeeded at all, it was an enormous pain. I don't see much benefit from unit testing it either: we can trust that the GLib functions do what they're supposed to do, as long as the surrounding logic is correct.

But this actually resolves all the Coverity issues on my laptop. There is a suppression for rand(), and there are two suppressions in crm_resource.c that will go away as part of my crm_resource overhaul PRs. And that's it.

nrwahl2 · 2025-07-02T09:14:34Z

Rebased on main with minor changes. I have not done any further testing or review.

daemons/fenced/fenced_commands.c

daemons/execd/remoted_pidone.c

clumens · 2025-07-02T16:00:55Z

daemons/execd/remoted_pidone.c

        while (isspace(*end)) {
            end++;
        }

-        if ((*end != '\0') && (*end != '#')) {
+        if (*end != '\0') {


This kind of function is the perfect candidate for unit tests, though it's hiding somewhere inaccessible at the moment and I would hate to expose it even as private API.

Yeah, we could make it library-private for that purpose. It is really annoying to me that static functions can't be unit-tested by cmocka AFAICT.

Yeah that's just an artifact of symbols not being visible if they're static. If you really want to test a static function, you can wrap the static bit in #if !defined(PCMK__UNIT_TESTING). We could even go so far as to define something like PCMK__STATIC (man I wish we had namespacing) to do that for us. libcrmcommon at least is already set up to build a whole separate version of the library for unit testing, so we can do whatever we want. This would be harder with daemons.

I've kept all the intermediate commits (with some very slight edits/fixes). But I added another commit on top that uses g_shell_parse_argv() and makes this stuff look almost trivial. I also added two commits early on that change the behavior of pcmk__scan_nvpair() somewhat.

Note that delay_max != delay_base implies that delay_max > delay_base. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Add 1 to the modulus so that we include the full range of (delay_max - delay_base), not that it really matters. For example, if delay_max == 5 and delay_base == 2, then we want to add values in {0, 1, 2, 3} to delay_base -- not to add values only in {0, 1, 2}. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

The current callers won't have trailing newlines from the CLI or from reading from a file. Newlines would be present only if the user included them explicitly. Newline removal was (re-)added by b7fe638, to fix a regression in 28ca6aa. However, at the time of 28ca6aa, pcmk_scan_nvpair() was public API, so the change in behavior actually mattered. Now, it doesn't. It appears to be safe to stop stripping trailing newlines. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

I don't see any reason not to. This does change behavior for crm_resource and stonith_admin if "NAME=" is given as a command line argument. However, this is such an edge case that I don't expect it to matter. It seems foolish to depend on the current behavior. Of course, that means nothing, so if anyone complains, we can revert. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 · 2025-07-13T00:47:54Z

daemons/execd/remoted_pidone.c

@@ -156,11 +157,17 @@ load_env_vars(void)
         */
        value_end = end;

+        // Strip trailing comment beginning with '#'
+        comment = strchr(end, '#');


Technically this is wrong in the case of, say, name=value# asdf. It turns out Bash sets name to "value#", not "value". So the asdf would be treated as a second token, not as part of a comment, and the whole line would be invalid.

Using g_shell_parse_argv() in a later commit fixes that among other things.

Better than defining our own swab functions if they're not available on the system. GLib already did that work for us and uses arch-specific optimizations where possible. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Coverity has been complaining about the payload_offset and payload_uncompressed being tainted scalar values. There's only so much validation we can do when we're reading from a socket. But apparently this is enough, because it makes the Coverity errors go away. There's a lot more room for improvement in the remote message processing. I found a few bugs a while back that we need to fix involving multiple messages received in rapid succession. This is an improvement for now. Note that I got rid of the CRM_LOG_ASSERT() line that subtracts 1 from the index. As far as I can tell, that's an off-by-one error and we have no reason to expect that position to contain a null byte. The commit that added it doesn't have any information in the commit message or comments. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Logic doesn't change otherwise. We just have to copy the "eat the rest of the line" bit. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

getline() allocates a buffer large enough to hold the entire input line, avoiding the need to worry about LINE_MAX (which glibc doesn't enforce anyway). The man page says: "getline() reads an entire line from stream, storing the address of the buffer containing the text into *lineptr. The buffer is null-terminated and includes the newline character, if one was found." So for each line that we read, either it contains a newline or we've reached EOF. This makes the line-eating and a bit of the error-checking unnecessary. Also define a constant for the file name instead of taking an argument. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Skipping (or now, stripping) leading whitespace is one fewer thing we have to do ourselves. g_strchug() is defined to operate on a (gchar *), but it modifies its argument in place without allocating/rellocating/freeing memory, so it's safe. Since find_env_var_name() only needs to return one pointer now, it doesn't need output arguments. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

After recent refactors, this is no longer necessary. We can just continue to the next outer loop iteration in the one place where it would have mattered. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

We consider the value invalid if anything besides whitespace or a comment (beginning with '#') follows it on the line. Further, we ignore all trailing whitespace after a valid value. So it's fine to strip all trailing whitespace, including a newline if present, before processing. Clearly we must now look for '\0' where we previously looked for '\n'. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

We don't want to do this until after parsing the value, because a '#' character inside a quoted value should be treated as part of the value. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

By stripping the rest of the trailing whitespace after stripping the comment, we eliminate the need to null-terminate explictly. If the value is valid (not followed by any garbage), then end already points to a terminating null byte after stripping the comment and remaining whitespace. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

It's no longer needed outside this block. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

The changes in this commit involve using (gchar *) strings and some dynamic allocation to make load_env_vars() easier to reason about. Previously, we tried to be as efficient as possible by reading a line into a buffer, iterating over it with multiple pointers, dividing it into name and value by insertion of a null terminator, etc. This is premature optimization. This function is not called along a path of execution where performance is so critical as to require this. In my opinion, clarity is more important. * Use pcmk__scan_nvpair() to separate the line on the equal sign. * Simplify name validation since we now have name as a separate string (before the equal sign) and don't need to return first and last pointers. * Skip the leading quote by converting it to a space and calling g_strchug() to memmove() value up by one position. This ensures that value still points to the beginning of the buffer, which simplifies freeing it later. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Our bespoke code has been fragile and hard to understand at a glance. There were certainly corner cases involving nested quotes that were not handled in the way the shell would handle them. Now we use g_shell_parse_argv() instead, to mimic how the shell would process the value in a "NAME=VALUE" assignment statement. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

At least on my machine, this one is no longer needed after all the recent refactors. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 requested a review from clumens March 3, 2025 09:52

nrwahl2 force-pushed the nrwahl2-coverity branch from 883c023 to 8481902 Compare March 3, 2025 09:55

clumens reviewed Mar 3, 2025

View reviewed changes

daemons/execd/remoted_pidone.c Outdated Show resolved Hide resolved

clumens added the review: in progress PRs that are currently being reviewed label Mar 3, 2025

nrwahl2 force-pushed the nrwahl2-coverity branch 2 times, most recently from 3ebf24e to 6a42764 Compare March 3, 2025 19:18

clumens mentioned this pull request Mar 5, 2025

Fix systemd unit overrides #3832

Merged

nrwahl2 force-pushed the nrwahl2-coverity branch from 6a42764 to cfbf4a4 Compare March 6, 2025 10:39

clumens reviewed Mar 6, 2025

View reviewed changes

daemons/execd/remoted_schemas.c Outdated Show resolved Hide resolved

nrwahl2 force-pushed the nrwahl2-coverity branch from cfbf4a4 to c94342c Compare March 8, 2025 00:57

nrwahl2 force-pushed the nrwahl2-coverity branch 5 times, most recently from ec1ad9f to f71ef81 Compare March 8, 2025 01:26

nrwahl2 marked this pull request as draft March 8, 2025 01:28

nrwahl2 force-pushed the nrwahl2-coverity branch 3 times, most recently from 81b5104 to d561a8b Compare March 8, 2025 01:34

nrwahl2 marked this pull request as ready for review March 8, 2025 06:59

nrwahl2 force-pushed the nrwahl2-coverity branch from f1b55a0 to 7e60792 Compare March 8, 2025 07:02

nrwahl2 force-pushed the nrwahl2-coverity branch from 7e60792 to 5ba0f17 Compare July 2, 2025 09:13

nrwahl2 changed the title ~~Some coverity fixes~~ pacemaker-remoted load_env_var refactors, and assorted minor changes Jul 2, 2025

clumens reviewed Jul 2, 2025

View reviewed changes

nrwahl2 added 4 commits July 9, 2025 22:42

Refactor: fencer: Simplify start_delay calculation for fence command

39dabdd

Note that delay_max != delay_base implies that delay_max > delay_base. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 force-pushed the nrwahl2-coverity branch 2 times, most recently from 8b610b7 to 3c18219 Compare July 13, 2025 00:35

nrwahl2 commented Jul 13, 2025

View reviewed changes

nrwahl2 added 14 commits July 12, 2025 17:58

Refactor: libcrmcommon: Use GLib functions for swapping byte order

c5b1608

Better than defining our own swab functions if they're not available on the system. GLib already did that work for us and uses arch-specific optimizations where possible. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: remoted: Remove unnecessary nesting in load_env_vars()

2330595

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: remoted: Remove more nesting from load_env_vars()

76c8306

Logic doesn't change otherwise. We just have to copy the "eat the rest of the line" bit. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: remoted: Skip setting value = NULL in load_env_vars()

21b7c5f

After recent refactors, this is no longer necessary. We can just continue to the next outer loop iteration in the one place where it would have mattered. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: remoted: Remove some nesting in load_env_vars()

71426e5

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: remoted: Strip trailing comment in load_env_vars()

8c0f2d3

We don't want to do this until after parsing the value, because a '#' character inside a quoted value should be treated as part of the value. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: remoted: Limit quote scope in load_env_vars()

1a1df60

It's no longer needed outside this block. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

Refactor: remoted: Functionize loading one environment variable

9e34af8

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

nrwahl2 force-pushed the nrwahl2-coverity branch from 3c18219 to cf12b21 Compare July 13, 2025 00:58

nrwahl2 force-pushed the nrwahl2-coverity branch from cf12b21 to f94c34d Compare July 13, 2025 01:25

Refactor: libcrmcommon: Drop unnecessary Coverity suppression

eebcadc

At least on my machine, this one is no longer needed after all the recent refactors. Signed-off-by: Reid Wahl <nrwahl@protonmail.com>

pacemaker-remoted load_env_var refactors, and assorted minor changes #3833

Are you sure you want to change the base?

pacemaker-remoted load_env_var refactors, and assorted minor changes #3833

Conversation

nrwahl2 commented Mar 3, 2025

Uh oh!

Uh oh!

nrwahl2 commented Mar 3, 2025

Uh oh!

clumens commented Mar 3, 2025

Uh oh!

nrwahl2 commented Mar 3, 2025

Uh oh!

nrwahl2 commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clumens commented Mar 5, 2025

Uh oh!

nrwahl2 commented Mar 6, 2025

Uh oh!

nrwahl2 commented Mar 6, 2025

Uh oh!

Uh oh!

nrwahl2 commented Mar 8, 2025

Uh oh!

nrwahl2 commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrwahl2 commented Jul 2, 2025

Uh oh!

Uh oh!

Uh oh!

clumens Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

nrwahl2 Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

clumens Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

nrwahl2 Jul 13, 2025

Choose a reason for hiding this comment

Uh oh!

nrwahl2 Jul 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nrwahl2 commented Mar 5, 2025 •

edited

Loading

nrwahl2 commented Mar 8, 2025 •

edited

Loading

nrwahl2 Jul 13, 2025 •

edited

Loading