Skip to content

fix(RuntimeProfile): Fix false deduplication of tokens that are a suffix of last token.#584

Merged
stefanberger merged 1 commit into
stefanberger:masterfrom
IWillBurn:FixRuntimeProfileDedupStrItems
Jun 1, 2026
Merged

fix(RuntimeProfile): Fix false deduplication of tokens that are a suffix of last token.#584
stefanberger merged 1 commit into
stefanberger:masterfrom
IWillBurn:FixRuntimeProfileDedupStrItems

Conversation

@IWillBurn

@IWillBurn IWillBurn commented May 28, 2026

Copy link
Copy Markdown
Contributor

Fix false deduplication of tokens that are a suffix of last token.

When processing a comma-separated list containing <token> at an arbitrary location and other-<token> at the end of the list, the RuntimeProfileDedupStrItems function incorrectly deletes <token> as a duplicate.

The root cause is in the boundary check when a strstr found match at the end of the string:

if (((dup[-1] == ',' || dup[-1] == 0) && dup[slen] == exp) ||
     (dup[slen] == 0) /* last item in list */)

When <token> is matched as a substring inside other-<token> (the last item), the (dup[slen] == 0) branch triggers without verifying that the left boundary of the match is valid — i.e. that it is preceded by a , or start of string. This causes <token> to be incorrectly deduplicated out of the list.

The fix tightens the boundary check to require a valid left-side delimiter in all cases:

if ((dup[-1] == ',' || dup[-1] == 0) &&
     (dup[slen] == exp || dup[slen] == 0))

This ensures a match is only treated as a true duplicate when both the left boundary and the right boundary are valid.

Summary by CodeRabbit

  • Bug Fixes
    • Improved comma-separated list deduplication: duplicates are now removed only when an item has valid boundaries on both sides, preventing partial-match edge cases and reducing incorrect removals when processing in-place comma-delimited values.

@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2eee98ef-cee0-496c-b3f7-2aef9335df79

📥 Commits

Reviewing files that changed from the base of the PR and between 8d3cfcd and 4909888.

📒 Files selected for processing (1)
  • src/tpm2/RuntimeProfile.c
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/tpm2/RuntimeProfile.c

📝 Walkthrough

Walkthrough

The PR tightens boundary validation in the comma-list deduplication function RuntimeProfileDedupStrItems, changing the "whole token" duplicate check to require both left-boundary and right-boundary conditions before removing duplicates.

Changes

Cohort / File(s) Summary
Deduplication Token Boundary
src/tpm2/RuntimeProfile.c
The condition that validates whether a found duplicate substring is a correctly delimited token is refactored to enforce left boundary (dup[-1] is , or string start) and right boundary (dup[slen] matches expected separator/terminator or string end) together before performing the memmove removal.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I nibble commas in the moonlight's glow,
Left and right I check each tiny row,
No stray token slips the careful seam,
Duplicates vanish like a dream,
A tidy list, hopped through by a rabbit's beam. 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary fix: preventing false deduplication of tokens that are suffixes of the final token, which matches the core bug addressed in the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@IWillBurn IWillBurn force-pushed the FixRuntimeProfileDedupStrItems branch from 048cda5 to a55ebcb Compare May 28, 2026 10:30
@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown

Actionable comments posted: 0

1 similar comment
@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown

Actionable comments posted: 0

@IWillBurn IWillBurn changed the title fix(RuntimeProfile): fix false dedup of tokens that are a suffix of a… fix(RuntimeProfile): fix false dedup May 28, 2026
@stefanberger

Copy link
Copy Markdown
Owner

Can you update your patch description and state in which case the problem occurred. This is useful for later on as well.

So I guess in your case you have this here as you said: "...,gost3410,ecc-tc26-gost3410"

The deduplication algo looks at gost3410 and tries to find a duplicate of this string following it. It finds a strstr-match inside ecc-tc26-gost3410. Because ecc-tc26-gost3410 happens to be the last one in the string we do not succeed on comparing with (dup[-1] == ',' || dup[-1] == 0) && dup[slen] == exp) but we go by (dup[slen] == 0) [last one in string is followed by \0]. Ok, I can see that this is a bug.

Your fix:

               if ((dup[-1] == ',' || dup[-1] == 0) &&
                    (dup[slen] == exp || dup[slen] == 0)) {

With your fix it wouldn't match the dup[-1] and we don't even need to look at the check after the &&. Alright, I agree, this is a good fix. Thanks.

@IWillBurn

IWillBurn commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Yes, that's right, that's the problem. Is there anything else that needs to be done to get the PR accepted? It seems "coverage/coveralls" is not working, maybe it should be restarted or something else.

@stefanberger

Copy link
Copy Markdown
Owner

Is there anything else that needs to be done to get the PR accepted?

A description of what the patch fixes would be good.

@IWillBurn IWillBurn changed the title fix(RuntimeProfile): fix false dedup fix(RuntimeProfile): Fix false deduplication of tokens that are a suffix of last token. Jun 1, 2026
@IWillBurn

Copy link
Copy Markdown
Contributor Author

Done.

@stefanberger

Copy link
Copy Markdown
Owner

Done.

The text should go into the patch description.

@IWillBurn IWillBurn force-pushed the FixRuntimeProfileDedupStrItems branch from a55ebcb to 8d3cfcd Compare June 1, 2026 14:28
Fix a bug where RuntimeProfileDedupStrItems incorrectly removes a
token that is a suffix of the last token in a comma-separated list.
For example, given "...,<token>,...,other-<token>", the function
would wrongly deduplicate "<token>" out of the list.

The root cause is the (dup[slen] == 0) branch in the boundary check,
which handles the last item in the list but does not verify that the
left boundary of the match is valid:

    if (((dup[-1] == ',' || dup[-1] == 0) && dup[slen] == exp) ||
         (dup[slen] == 0) /* last item in list */)

The fix requires a valid left-side delimiter in all cases:

    if ((dup[-1] == ',' || dup[-1] == 0) &&
         (dup[slen] == exp || dup[slen] == 0))

Signed-off-by: IWillBurn <eugene.boyarnikov@gmail.com>
@IWillBurn IWillBurn force-pushed the FixRuntimeProfileDedupStrItems branch from 8d3cfcd to 4909888 Compare June 1, 2026 14:32
@stefanberger stefanberger merged commit 73c3e76 into stefanberger:master Jun 1, 2026
5 checks passed
@stefanberger

Copy link
Copy Markdown
Owner

Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants