Skip to content

ENT-3417: Added allow_non_convergent to pattern_matching promise in edit_line#6025

Open
victormlg wants to merge 2 commits intocfengine:masterfrom
victormlg:replace_pattern
Open

ENT-3417: Added allow_non_convergent to pattern_matching promise in edit_line#6025
victormlg wants to merge 2 commits intocfengine:masterfrom
victormlg:replace_pattern

Conversation

@victormlg
Copy link
Contributor

@victormlg victormlg commented Jan 26, 2026

Added a new attribute allow_non_convergent in replace_pattern promise to allow for idempotent non-convergent regex.

bundle agent __main__
{
  files:
      "/tmp/example.txt"
        content => "bash PORT=23 echo";

      "/tmp/example.txt"
        edit_line => regex_replace( "PORT=[0-9]+", "PORT=22" );

}
# From the stdlib:
bundle edit_line regex_replace(find,replace)
# @brief Find exactly a regular expression and replace exactly the match with a string.
# You can think of this like a PCRE powered sed.
# @param find The regular expression
# @param replace The replacement string
{
  replace_patterns:

      "$(find)"
        replace_with => value("$(replace)"),
        comment => "Search and replace string",
        allow_non_convergent => "true";
}

body replace_with value(x)
# @brief Replace matching lines
# @param x The replacement string
{
        replace_value => "$(x)";
        occurrences => "all";
}

I am a bit ensure on the implementation, but the idea is that we don't want to allow destructive non-convergent regex, such as:

bundle agent __main__
{
  files:
      "/tmp/example.txt"
        content => "PORT=23";

      "/tmp/example.txt"
        edit_line => regex_replace( "PORT=[0-9]+", "PORT=22 #Some comment" );

}

This will replace and rematch the same line indefinitely:
PORT=22 #Some comment #Some comment #Some comment [...]

So the idea was to let cfengine match and expand the pattern N times (as before), then we match and expand one more time (N+1), and if the total length of the line is greater, this means that the non-convergent regex is destructive and thus we need to error.

@victormlg victormlg requested a review from larsewi January 26, 2026 17:49
@victormlg victormlg changed the title ENT-3417: Added use_non_convergent to pattern_matching promise in edit_line ENT-3417: Added allow_non_convergent to pattern_matching promise in edit_line Jan 27, 2026
Copy link
Contributor

@larsewi larsewi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an acceptance test :)

@larsewi larsewi requested a review from nickanderson January 29, 2026 11:46
@larsewi
Copy link
Contributor

larsewi commented Jan 29, 2026

@nickanderson can you address the questions @victormlg posted in the PR description?

Ticket: ENT-3417
Signed-off-by: Victor Moene <victor.moene@northern.tech>
Signed-off-by: Victor Moene <victor.moene@northern.tech>
int needed = snprintf(line_buff_cp + start_off, sizeof(line_buff_cp) - start_off,
"%s%s", BufferData(replace), after);

if (needed >= sizeof(line_buff_cp) - start_off) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure needed is not a negative integer before comparing it with an unsigned

Comment on lines +1337 to +1338
strlcpy(line_buff_cp, line_buff, sizeof(line_buff_cp));
strlcpy(after, line_buff_cp + end_off, sizeof(after));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check for truncation

"%s%s", BufferData(replace), after);

if (needed >= sizeof(line_buff_cp) - start_off) {
RecordInterruption(ctx, pp, a, "Buffer overflow: replacement string is too large. '%s' in '%s'",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe avoid printing buffer overflow. Because you are avoiding it. The remaining message is sufficient in my opinion


######################################################

bundle agent test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are including default.cf.sub you can add the meta data with ticket number and description

@nickanderson
Copy link
Member

nickanderson commented Feb 4, 2026

@larsewi

@nickanderson can you address the questions @victormlg posted in the PR description?

Sorry, I had responded to @victormlg directly via slack.

I don't love the name use_non_convergent. Maybe allow_non_convergent? Semantically it's something we are allowing not a thing we are using.

I am unsure about the non-descructive thing.

Maybe it should be boxed in more specifically ... the use case is for the pattern similar to:

sed -i 's/PORT=.*/PORT=22/'

Basically, I don't know what is there now, but it should be PORT=22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants