feat(app-launch-helper): normalize ExecSearchPath before sending to D… #316
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideNormalizes and validates ExecSearchPath values before sending them over D-Bus by introducing a dedicated normalization function and wiring it into the property-serialization path. Sequence diagram for processing ExecSearchPath propertysequenceDiagram
participant processKVPair
participant normalizeExecSearchPath
participant sd_bus_message as sd_bus_message
participant sd_journal as sd_journal
processKVPair->>processKVPair: Iterate over props map
alt key is ExecSearchPath
processKVPair->>normalizeExecSearchPath: normalizeExecSearchPath(value)
normalizeExecSearchPath-->>processKVPair: normalizedStorage (vector<string>)
processKVPair->>processKVPair: Build normalizedValue list from normalizedStorage
alt normalizedValue is empty
processKVPair->>sd_journal: LOG_WARNING ExecSearchPath normalized to empty
processKVPair-->>processKVPair: Continue to next property (skip serialization)
else normalizedValue not empty
processKVPair-->>processKVPair: valuePtr = &normalizedValue
end
else key is not ExecSearchPath
processKVPair-->>processKVPair: valuePtr = &value (original)
end
processKVPair->>sd_bus_message: sd_bus_message_open_container(struct sv)
processKVPair->>sd_bus_message: sd_bus_message_append_basic(key)
processKVPair->>sd_bus_message: sd_bus_message_open_container(variant)
processKVPair->>sd_bus_message: appendPropValue(getPropType(key), *valuePtr)
processKVPair->>sd_bus_message: sd_bus_message_close_container(variant)
processKVPair->>sd_bus_message: sd_bus_message_close_container(struct)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new ExecSearchPath normalization path goes through a temporary
std::vector<std::string>and thenstd::list<std::string_view>; double-check thatappendPropValueand the D-Bus marshalling do not retain thesestring_views beyond the scope ofprocessKVPair, as they now point to short‑lived strings rather than the original backing storage. - In
normalizeExecSearchPath, consider reserving capacity forresultanddedupbased onvalue.size()to avoid repeated allocations when normalizing larger ExecSearchPath lists.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new ExecSearchPath normalization path goes through a temporary `std::vector<std::string>` and then `std::list<std::string_view>`; double-check that `appendPropValue` and the D-Bus marshalling do not retain these `string_view`s beyond the scope of `processKVPair`, as they now point to short‑lived strings rather than the original backing storage.
- In `normalizeExecSearchPath`, consider reserving capacity for `result` and `dedup` based on `value.size()` to avoid repeated allocations when normalizing larger ExecSearchPath lists.
## Individual Comments
### Comment 1
<location> `apps/app-launch-helper/src/main.cpp:168-175` </location>
<code_context>
+
+ // Reject control characters (e.g. \n, \t, \0). Align closer to systemd.
+ bool hasCtrl = false;
+ for (char ch : v) {
+ if (std::iscntrl(static_cast<unsigned char>(ch))) {
+ hasCtrl = true;
+ break;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider whether rejecting all control characters is too strict or locale-dependent for valid paths.
`std::iscntrl` is locale-dependent and will reject any locale-specific control characters across the entire path. On systems using non-UTF-8 or other encodings, this may reject paths that the filesystem accepts. If you only want ASCII control characters, consider checking `ch < 0x20 || ch == 0x7f` instead to avoid locale dependence and clarify the intended policy.
```suggestion
// Reject ASCII control characters (e.g. \n, \t, \0) to align closer to systemd and avoid locale-dependent behavior.
bool hasCtrl = false;
for (unsigned char ch : v) {
if (ch < 0x20 || ch == 0x7f) {
hasCtrl = true;
break;
}
}
```
</issue_to_address>
### Comment 2
<location> `apps/app-launch-helper/src/main.cpp:248-255` </location>
<code_context>
+ std::list<std::string_view> normalizedValue;
+ std::vector<std::string> normalizedStorage;
+
+ if (key == "ExecSearchPath") {
+ normalizedStorage = normalizeExecSearchPath(value);
+ for (const auto &s : normalizedStorage) {
+ normalizedValue.emplace_back(s);
+ }
+
+ if (normalizedValue.empty()) {
+ sd_journal_print(LOG_WARNING, "ExecSearchPath normalized to empty, skipping property");
+ continue;
+ }
</code_context>
<issue_to_address>
**suggestion (performance):** Multiple journal logs per malformed `ExecSearchPath` entry may be noisy in real deployments.
With `LOG_NOTICE` in `normalizeExecSearchPath` plus this `LOG_WARNING`, a single bad or frequently-used `ExecSearchPath` could flood the journal. If this code runs often (e.g., every launch), consider reducing the per-entry log level, aggregating messages, or adding rate limiting to avoid excessive log noise.
Suggested implementation:
```cpp
if (key == "ExecSearchPath") {
normalizedStorage = normalizeExecSearchPath(value);
for (const auto &s : normalizedStorage) {
normalizedValue.emplace_back(s);
}
if (normalizedValue.empty()) {
/* Log at most once per process to avoid flooding the journal on frequently-used bad ExecSearchPath values. */
static bool exec_search_path_empty_logged = false;
if (!exec_search_path_empty_logged) {
sd_journal_print(LOG_NOTICE, "ExecSearchPath normalized to empty, skipping property (further occurrences will be suppressed)");
exec_search_path_empty_logged = true;
}
continue;
}
```
You also mentioned `LOG_NOTICE` usage inside `normalizeExecSearchPath`. To fully address log noise:
1. Locate any `sd_journal_print(LOG_NOTICE, ...)` calls inside `normalizeExecSearchPath` and either:
- Downgrade them to `LOG_DEBUG` or `LOG_INFO`, or
- Guard them with a similar static “logged once” flag, or
- Aggregate them (e.g., count invalid entries and log a single summary message per call).
2. If `normalizeExecSearchPath` may be called in tight loops or for many entries, prefer either aggregation per call or DEBUG-level logs to minimize noise in production.
Because the body of `normalizeExecSearchPath` is not fully visible in the snippet, those adjustments must be made where its logging statements are defined.
</issue_to_address>
### Comment 3
<location> `apps/app-launch-helper/src/main.cpp:209` </location>
<code_context>
+ return result;
+}
+
int appendPropValue(msg_ptr &msg, DBusValueType type, const std::list<std::string_view> &value)
{
int ret;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the ExecSearchPath handling by moving container adaptation into an appendPropValue overload and splitting normalization into small helpers to simplify control flow and improve readability.
You can keep the behavior but significantly reduce the “container dance” and control-flow indirection around `ExecSearchPath` by:
1. **Localizing the container juggling inside `appendPropValue` via an overload**, instead of in `processKVPair`.
2. **Splitting `normalizeExecSearchPath` into small helpers** to make it easier to read/maintain.
### 1. Simplify `processKVPair` by adding an overload of `appendPropValue`
Instead of `valuePtr`, `normalizedValue`, and `normalizedStorage` plus a pointer switch, push the adaptation into `appendPropValue`:
```cpp
int appendPropValue(msg_ptr &msg, DBusValueType type,
const std::list<std::string_view> &value);
int appendPropValue(msg_ptr &msg, DBusValueType type,
const std::vector<std::string> &value)
{
// Reuse existing implementation by adapting locally here
std::list<std::string_view> views;
views.reserve(value.size()); // if using a small helper container, keep it cheap
for (const auto &s : value) {
views.emplace_back(s);
}
return appendPropValue(msg, type, views);
}
```
Then `processKVPair` can be simplified to straight-line logic with two explicit branches and no pointer indirection:
```cpp
int processKVPair(msg_ptr &msg,
const std::map<std::string_view, std::list<std::string_view>> &props)
{
int ret;
if (!props.empty()) {
for (auto [key, value] : props) {
std::string keyStr{key};
if (ret = sd_bus_message_open_container(msg, SD_BUS_TYPE_STRUCT, "sv"); ret < 0) {
sd_journal_perror("open struct of properties failed.");
return ret;
}
if (ret = sd_bus_message_append(msg, "s", keyStr.data()); ret < 0) {
sd_journal_perror("append key of property failed.");
return ret;
}
if (key == "ExecSearchPath") {
std::vector<std::string> normalized = normalizeExecSearchPath(value);
if (normalized.empty()) {
sd_journal_print(LOG_WARNING,
"ExecSearchPath normalized to empty, skipping property");
// Skip closing value, just close the struct
if (ret = sd_bus_message_close_container(msg); ret < 0) {
sd_journal_perror("close struct of properties failed.");
return ret;
}
continue;
}
if (ret = appendPropValue(msg, getPropType(key), normalized); ret < 0) {
sd_journal_perror("append value of property failed.");
return ret;
}
} else {
if (ret = appendPropValue(msg, getPropType(key), value); ret < 0) {
sd_journal_perror("append value of property failed.");
return ret;
}
}
if (ret = sd_bus_message_close_container(msg); ret < 0) {
sd_journal_perror("close struct of properties failed.");
return ret;
}
}
}
return 0;
}
```
This keeps all existing behavior but:
- Removes `valuePtr`, `normalizedValue`, and the implicit lifetime coupling between `normalizedStorage` and `normalizedValue`.
- Makes the ExecSearchPath special case explicit and easy to follow.
- Contains the `vector<string> -> list<string_view>` conversion in one place (`appendPropValue` overload), which is where that adaptation logically belongs.
### 2. Decompose `normalizeExecSearchPath` into small helpers
The normalization logic is correct but dense. Extracting small helpers reduces cognitive load without changing behavior:
```cpp
bool containsControlChars(std::string_view v)
{
for (char ch : v) {
if (std::iscntrl(static_cast<unsigned char>(ch))) {
return true;
}
}
return false;
}
std::string normalizePathString(std::string_view v)
{
std::filesystem::path p{std::string{v}};
auto norm = p.lexically_normal().string();
while (norm.size() > 1 && norm.back() == '/') {
norm.pop_back();
}
return norm;
}
bool isValidAbsolutePath(std::string_view v)
{
return !v.empty() && v.front() == '/';
}
```
Then `normalizeExecSearchPath` becomes much clearer:
```cpp
std::vector<std::string> normalizeExecSearchPath(
const std::list<std::string_view> &value)
{
std::vector<std::string> result;
std::unordered_set<std::string> dedup;
for (const auto &v : value) {
if (v.empty()) {
continue;
}
if (containsControlChars(v)) {
sd_journal_print(LOG_NOTICE, "ExecSearchPath skip control-chars present");
continue;
}
if (!isValidAbsolutePath(v)) {
sd_journal_print(LOG_NOTICE, "ExecSearchPath skip non-absolute: %.*s",
static_cast<int>(v.size()), v.data());
continue;
}
std::string norm = normalizePathString(v);
if (!isValidAbsolutePath(norm)) {
sd_journal_print(LOG_NOTICE,
"ExecSearchPath skip normalized non-absolute: %s",
norm.c_str());
continue;
}
if (!dedup.emplace(norm).second) {
continue;
}
result.emplace_back(std::move(norm));
}
return result;
}
```
All original checks and behavior remain, but each concern is in a named helper, which makes the overall feature easier to understand and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
pipx 为我PATH设置了 /home/deepin/.local/share/../bin |
There was a problem hiding this comment.
Pull request overview
This PR adds normalization and validation of the ExecSearchPath property before sending it to D-Bus in the app-launch-helper. The implementation ensures that only absolute, well-formed paths without control characters or ".." components are accepted, and duplicates are removed.
Key changes:
- Added
normalizeExecSearchPath()function to validate, normalize, and deduplicate path entries - Integrated path normalization into the D-Bus message preparation flow in
processKVPair() - Added validation to reject empty paths, non-absolute paths, paths with control characters, and paths that normalize to non-absolute
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/app-launch-helper/src/main.cpp
Outdated
Comment on lines
169
to
175
| bool hasCtrl = false; | ||
| for (char ch : v) { | ||
| if (std::iscntrl(static_cast<unsigned char>(ch))) { | ||
| hasCtrl = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider extracting the control character detection into a separate helper function or using std::any_of for better readability and to follow functional programming patterns used elsewhere in the codebase.
apps/app-launch-helper/src/main.cpp
Outdated
| } | ||
| } | ||
| if (hasCtrl) { | ||
| sd_journal_print(LOG_NOTICE, "ExecSearchPath skip control-chars present"); |
There was a problem hiding this comment.
The error message should include the actual path that was skipped to aid in debugging. Consider logging which path entry contained control characters, similar to how non-absolute paths are logged on line 182.
Suggested change
| sd_journal_print(LOG_NOTICE, "ExecSearchPath skip control-chars present"); | |
| sd_journal_print(LOG_NOTICE, "ExecSearchPath skip control-chars present: %.*s", | |
| static_cast<int>(v.size()), v.data()); |
wineee
force-pushed
the
fix
branch
3 times, most recently
from
4fec418 to
e8cccdd
Compare
…-Bus Log: ExecSearchPath must be absolute and must not contain a ".." path component
deepin pr auto review我来对这个diff进行详细的代码审查：
可改进之处：
改进建议： // 建议抽取为独立函数
std::list<std::string_view> normalizeExecSearchPaths(const std::list<std::string_view>& value) {
std::vector<std::string> normalizedStorage;
normalizedStorage.reserve(value.size()); // 预分配空间
for (const auto& v : value) {
std::filesystem::path p{std::string{v}};
if (!p.is_absolute()) {
sd_journal_print(LOG_INFO, "ExecSearchPath ignoring relative path: %s", std::string{v}.c_str());
continue;
}
normalizedStorage.emplace_back(p.lexically_normal().string());
}
if (normalizedStorage.empty()) {
sd_journal_print(LOG_WARNING, "ExecSearchPath normalized to empty, skipping property");
return {};
}
return std::list<std::string_view>(normalizedStorage.begin(), normalizedStorage.end());
}
// 在主函数中使用
if (key == "ExecSearchPath") {
normalizedValue = normalizeExecSearchPaths(value);
if (normalizedValue.empty()) {
continue;
}
valuePtr = &normalizedValue;
}这个改进版本：
|
zccrs
requested changes
Dec 24, 2025
zccrs
approved these changes
Dec 24, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wineee, zccrs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…-Bus
Log: ExecSearchPath must be absolute and must not contain a ".." path component
Summary by Sourcery
Normalize and validate the ExecSearchPath property before sending it over D-Bus to ensure only absolute, well-formed paths are used.
Bug Fixes:
Enhancements: