Skip to content

mavros: validate plugin list patterns#2192

Merged
vooon merged 2 commits into
mavlink:ros2from
apples-kksk:fix-plugin-allowlist-validation
May 14, 2026
Merged

mavros: validate plugin list patterns#2192
vooon merged 2 commits into
mavlink:ros2from
apples-kksk:fix-plugin-allowlist-validation

Conversation

@apples-kksk
Copy link
Copy Markdown
Contributor

Requested

Issue #2024 reports that plugin_allowlist accepts garbage input without reporting an error.

Implemented

  • Validate plugin_allowlist and plugin_denylist patterns against declared plugin classes before loading plugins.
  • Preserve existing glob matching behavior for valid exact and wildcard patterns.
  • Add unit coverage for valid patterns and invalid allowlist/denylist values.

Not covered

No changes to plugin loading order or plugin implementations.

Validated

  • git diff --check HEAD~1..HEAD
  • Attempted cmake -S mavros -B /tmp/mavros-2024-cmake-check, but this environment is missing ament_cmake.

colcon and ament_uncrustify are not available in the current environment.

Copy link
Copy Markdown
Member

@vooon vooon left a comment

Choose a reason for hiding this comment

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

I think it's a bad idea crashing if something in a list doesn't match.
I think better just to emit warning.

Comment thread mavros/src/lib/uas_executor.cpp Outdated
@apples-kksk apples-kksk force-pushed the fix-plugin-allowlist-validation branch from a5bdc13 to 084b256 Compare May 13, 2026 07:44
@apples-kksk
Copy link
Copy Markdown
Contributor Author

Updated, thanks for the review.\n\n- Rebased the branch so the #2189 executor-thread changes are no longer included in this PR.\n- Changed unmatched plugin allow/deny patterns to emit a warning instead of throwing/crashing.\n\nI also checked the final PR diff: it now only touches mavros/src/lib/mavros_uas.cpp.

Copy link
Copy Markdown
Member

@vooon vooon left a comment

Choose a reason for hiding this comment

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

Now looks good, but why not to use lambda?

@vooon vooon added this to the Version 2.15 milestone May 13, 2026
@apples-kksk
Copy link
Copy Markdown
Contributor Author

apples-kksk commented May 13, 2026

No strong reason. I used a small named helper to keep the startup block readable and reuse the same logic for both plugin_allowlist and plugin_denylist.

A local lambda would work too; I can switch to that if you prefer.

@vooon
Copy link
Copy Markdown
Member

vooon commented May 13, 2026

Yes, it likely be cleaner.

@apples-kksk apples-kksk force-pushed the fix-plugin-allowlist-validation branch from 084b256 to a625e5e Compare May 13, 2026 09:35
@apples-kksk apples-kksk force-pushed the fix-plugin-allowlist-validation branch from a625e5e to d1408fc Compare May 13, 2026 09:38
@vooon
Copy link
Copy Markdown
Member

vooon commented May 13, 2026

@apples-kksk
Copy link
Copy Markdown
Contributor Author

Applied the uncrustify formatting in 9966b4b. Thanks.

@vooon vooon merged commit 16525ba into mavlink:ros2 May 14, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants