Skip to content

fix(DUP-001): CU-86akhf8u2 9 review findings across 4 files #100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

fix(DUP-001): CU-86akhf8u2 9 review findings across 4 files #100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0eecbd9
fix(DUP-001): 9 review findings across 4 files
flamingo[bot] Sep 28, 2026
a05cfd6
fix(DUP-001): 9 review findings across 4 files
flamingo[bot] Sep 28, 2026
ed21cd9
fix(DUP-001): 9 review findings across 4 files
flamingo[bot] Sep 28, 2026
e70c1d2
fix(DUP-001): 9 review findings across 4 files
flamingo[bot] Sep 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 25 additions & 26 deletions osquery/filesystem/filesystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,26 @@ Status checkFileReadLimit(std::size_t file_size,

return Status::success();
}

Status checkPathAccess(const fs::path& path,
bool effective,
int open_mode,
int access_mode,
const char* denied_message) {
auto path_exists = pathExists(path);
if (!path_exists.ok()) {
return path_exists;
}

if (effective) {
PlatformFile fd(path, PF_OPEN_EXISTING | open_mode);
return Status(fd.isValid() ? 0 : 1);
} else if (platformAccess(path.string(), access_mode) == 0) {
return Status::success();
}

return Status(1, std::string(denied_message) + path.string());
}
} // namespace

Status writeTextFile(const fs::path& path,
Expand Down Expand Up @@ -242,35 +262,13 @@ Status readFile(const fs::path& path, std::string& content, bool shouldLog) {
}

Status isWritable(const fs::path& path, bool effective) {

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 osquery/filesystem/filesystem#isWritable duplicates an existing definition

isWritable (line ~245) now delegates to a new private helper checkPathAccess (defined in the anonymous namespace alongside checkFileReadLimit), passing PF_WRITE/W_OK and the "not writable" message, eliminating the duplicated body it shared with isReadable.

🤖 Prompt for AI agents 
In osquery/filesystem/filesystem.cpp around line 244, review and complete this code-review fix: osquery/filesystem/filesystem#isWritable duplicates an existing definition.
What the draft fix changed: `isWritable` (line ~245) now delegates to a new private helper `checkPathAccess` (defined in the anonymous namespace alongside `checkFileReadLimit`), passing `PF_WRITE`/`W_OK` and the "not writable" message, eliminating the duplicated body it shared with `isReadable`.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer

auto path_exists = pathExists(path);
if (!path_exists.ok()) {
return path_exists;
}

if (effective) {
PlatformFile fd(path, PF_OPEN_EXISTING | PF_WRITE);
return Status(fd.isValid() ? 0 : 1);
} else if (platformAccess(path.string(), W_OK) == 0) {
return Status::success();
}

return Status(1, "Path is not writable: " + path.string());
return checkPathAccess(
path, effective, PF_WRITE, W_OK, "Path is not writable: ");
}

Status isReadable(const fs::path& path, bool effective) {

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 osquery/filesystem/filesystem#isReadable duplicates an existing definition

isReadable (line ~250) now likewise delegates to the same checkPathAccess helper, passing PF_READ/R_OK and the "not readable" message, so both functions share one implementation instead of duplicating the pathExists/PlatformFile/platformAccess logic.

🤖 Prompt for AI agents 
In osquery/filesystem/filesystem.cpp around line 260, review and complete this code-review fix: osquery/filesystem/filesystem#isReadable duplicates an existing definition.
What the draft fix changed: `isReadable` (line ~250) now likewise delegates to the same `checkPathAccess` helper, passing `PF_READ`/`R_OK` and the "not readable" message, so both functions share one implementation instead of duplicating the pathExists/PlatformFile/platformAccess logic.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer

auto path_exists = pathExists(path);
if (!path_exists.ok()) {
return path_exists;
}

if (effective) {
PlatformFile fd(path, PF_OPEN_EXISTING | PF_READ);
return Status(fd.isValid() ? 0 : 1);
} else if (platformAccess(path.string(), R_OK) == 0) {
return Status::success();
}

return Status(1, "Path is not readable: " + path.string());
return checkPathAccess(
path, effective, PF_READ, R_OK, "Path is not readable: ");
}

Status pathExists(const fs::path& path) {
Expand Down Expand Up @@ -665,3 +663,4 @@ std::string lsperms(int mode) {
return bits;
}
} // namespace osquery

177 changes: 92 additions & 85 deletions osquery/tables/applications/posix/docker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,9 +525,26 @@ QueryData genContainerLabels(QueryContext& context) {
}

/**
* @brief Entry point for docker_container_mounts table.
* @brief Utility method shared by docker_container_mounts and
* docker_container_ports tables to iterate over containers and extract a
* child array of details into rows via the provided callback.
*
* @param context Query context.
* @param child_path Path of the child array node to iterate (e.g. "Mounts",
* "Ports").
* @param error_label Label used in the VLOG error message on failure.
* @param row_builder Callback invoked with the container node, the set of
* constrained ids, and the child node to populate a Row which is appended to
* results.
*/
QueryData genContainerMounts(QueryContext& context) {

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 osquery/tables/applications/posix/docker#osquery::tables::genContainerMounts duplicates a near-identical definition

genContainerMounts (formerly at line 530) was rewritten to call a new shared helper getContainerChildRows() (added above genContainerMounts), passing "Mounts" as the child path and a lambda that builds the mount-specific Row fields. This removes the near-duplicate loop/try/catch structure it shared with genContainerPorts. Needed #include <functional> transitively via existing headers (std::function used); if not already transitively included, this could fail to compile — worth a build check, hence not marked 90+.

🤖 Prompt for AI agents 
In osquery/tables/applications/posix/docker.cpp around line 530, review and complete this code-review fix: osquery/tables/applications/posix/docker#osquery::tables::genContainerMounts duplicates a near-identical definition.
What the draft fix changed: genContainerMounts (formerly at line 530) was rewritten to call a new shared helper `getContainerChildRows()` (added above genContainerMounts), passing "Mounts" as the child path and a lambda that builds the mount-specific Row fields. This removes the near-duplicate loop/try/catch structure it shared with genContainerPorts. Needed `#include <functional>` transitively via existing headers (std::function used); if not already transitively included, this could fail to compile — worth a build check, hence not marked 90+.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

QueryData getContainerChildRows(
QueryContext& context,
const std::string& child_path,
const std::string& error_label,
const std::function<void(const pt::ptree&,
const std::set<std::string>&,
const pt::ptree&,
Row&)>& row_builder) {
QueryData results;
std::set<std::string> ids;
pt::ptree containers;
Expand All @@ -539,9 +556,32 @@ QueryData genContainerMounts(QueryContext& context) {
for (const auto& entry : containers) {
const pt::ptree& container = entry.second;
try {
for (const auto& node : container.get_child("Mounts")) {
const pt::ptree& mount = node.second;
for (const auto& node : container.get_child(child_path)) {
const pt::ptree& child = node.second;
Row r;
row_builder(container, ids, child, r);
results.push_back(r);
}
} catch (const pt::ptree_error& e) {
VLOG(1) << "Error getting docker " << error_label << " " << e.what();
}
}

return results;
}

/**
* @brief Entry point for docker_container_mounts table.
*/
QueryData genContainerMounts(QueryContext& context) {
return getContainerChildRows(
context,
"Mounts",
"container mounts",
[](const pt::ptree& container,
const std::set<std::string>& ids,
const pt::ptree& mount,
Row& r) {
r["id"] = getValue(container, ids, "Id");
r["type"] = mount.get<std::string>("Type", "");
r["name"] = mount.get<std::string>("Name", "");
Expand All @@ -551,14 +591,7 @@ QueryData genContainerMounts(QueryContext& context) {
r["mode"] = mount.get<std::string>("Mode", "");
r["rw"] = (mount.get<bool>("RW", false) ? INTEGER(1) : INTEGER(0));
r["propagation"] = mount.get<std::string>("Propagation", "");
results.push_back(r);
}
} catch (const pt::ptree_error& e) {
VLOG(1) << "Error getting docker container mounts " << e.what();
}
}

return results;
});
}

/**
Expand Down Expand Up @@ -605,33 +638,20 @@ QueryData genContainerNetworks(QueryContext& context) {
* @brief Entry point for docker_container_ports table.
*/
QueryData genContainerPorts(QueryContext& context) {

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 osquery/tables/applications/posix/docker#osquery::tables::genContainerPorts duplicates a near-identical definition

genContainerPorts (formerly at line 607) was rewritten to call the same new shared helper getContainerChildRows(), passing "Ports" as the child path and a lambda that builds the port-specific Row fields, eliminating the duplicated iterate/try/catch structure previously copy-pasted from genContainerMounts.

🤖 Prompt for AI agents 
In osquery/tables/applications/posix/docker.cpp around line 607, review and complete this code-review fix: osquery/tables/applications/posix/docker#osquery::tables::genContainerPorts duplicates a near-identical definition.
What the draft fix changed: genContainerPorts (formerly at line 607) was rewritten to call the same new shared helper `getContainerChildRows()`, passing "Ports" as the child path and a lambda that builds the port-specific Row fields, eliminating the duplicated iterate/try/catch structure previously copy-pasted from genContainerMounts.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

QueryData results;
std::set<std::string> ids;
pt::ptree containers;
Status s = getContainers(context, ids, containers);
if (!s.ok()) {
return results;
}

for (const auto& entry : containers) {
const pt::ptree& container = entry.second;
try {
for (const auto& node : container.get_child("Ports")) {
const pt::ptree& details = node.second;
Row r;
return getContainerChildRows(
context,
"Ports",
"container ports",
[](const pt::ptree& container,
const std::set<std::string>& ids,
const pt::ptree& details,
Row& r) {
r["id"] = getValue(container, ids, "Id");
r["type"] = details.get<std::string>("Type", "");
r["port"] = INTEGER(details.get<int>("PrivatePort", 0));
r["host_ip"] = details.get<std::string>("IP", "");
r["host_port"] = INTEGER(details.get<int>("PublicPort", 0));
results.push_back(r);
}
} catch (const pt::ptree_error& e) {
VLOG(1) << "Error getting docker container ports " << e.what();
}
}

return results;
});
}

/**
Expand Down Expand Up @@ -1080,52 +1100,6 @@ void getImageLayers(const std::string& image_id, QueryData& results) {
}
}

/**
* @brief Calls layer extractor for all images for docker_image_layers table
*/
void getImageLayersAll(QueryData& results) {

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 osquery/tables/applications/posix/docker#osquery::tables::getImageLayersAll duplicates an existing definition

getImageLayersAll (formerly at line 1086) was removed and replaced by a call to a new shared helper getImageDetailsAll() (added just above genImageLayers) which fetches "/images/json", iterates entries, extracts the sha256-stripped image id, and invokes a caller-supplied per-image extractor function pointer (getImageLayers). genImageLayers now calls getImageDetailsAll(results, "image details", getImageLayers) in its else branch instead of the removed getImageLayersAll.

🤖 Prompt for AI agents 
In osquery/tables/applications/posix/docker.cpp around line 1086, review and complete this code-review fix: osquery/tables/applications/posix/docker#osquery::tables::getImageLayersAll duplicates an existing definition.
What the draft fix changed: getImageLayersAll (formerly at line 1086) was removed and replaced by a call to a new shared helper `getImageDetailsAll()` (added just above genImageLayers) which fetches "/images/json", iterates entries, extracts the sha256-stripped image id, and invokes a caller-supplied per-image extractor function pointer (getImageLayers). genImageLayers now calls `getImageDetailsAll(results, "image details", getImageLayers)` in its else branch instead of the removed getImageLayersAll.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

pt::ptree tree;
Status s = dockerApi("/images/json", tree);
if (!s.ok()) {
VLOG(1) << "Error getting docker images: " << s.what();
return;
}
for (const auto& entry : tree) {
try {
const pt::ptree& node = entry.second;
std::string id = node.get<std::string>("Id", "");
if (boost::starts_with(id, "sha256:")) {
id.erase(0, 7);
}
getImageLayers(id, results);
} catch (const pt::ptree_error& e) {
VLOG(1) << "Error getting docker image details: " << e.what();
}
}
}

/**
* @brief Entry point for docker_image_layers table.
*/
QueryData genImageLayers(QueryContext& context) {
QueryData results;
pt::ptree tree;
std::vector<std::string> layers;

if (context.constraints["id"].exists(
EQUALS)) { // get layers for specific image
for (const auto& id : context.constraints["id"].getAll(EQUALS)) {
if (!checkConstraintValue(id)) {
continue;
}
getImageLayers(id, results);
}
} else { // get layers for all images
getImageLayersAll(results);
}
return results;
}

/**
* @brief Image history extractor for docker_image_history table
*/
Expand Down Expand Up @@ -1163,9 +1137,19 @@ void getImageHistory(const std::string& image_id, QueryData& results) {
}

/**
* @brief Calls history for all images for docker_image_history table
* @brief Utility method shared by docker_image_layers and
* docker_image_history tables to invoke a per-image extractor function for
* every image reported by the docker API.
*
* @param results Results collection to populate.
* @param error_label Label used in the VLOG error message on failure.
* @param extractor Per-image extraction function (getImageLayers or
* getImageHistory).
*/
void getImageHistoryAll(QueryData& results) {

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 osquery/tables/applications/posix/docker#osquery::tables::getImageHistoryAll duplicates an existing definition

getImageHistoryAll (formerly at line 1168) was removed and replaced by a call to the same new shared helper getImageDetailsAll(), passing the getImageHistory function as the per-image extractor. genImageHistory now calls getImageDetailsAll(results, "image history", getImageHistory) in its else branch instead of the removed getImageHistoryAll, eliminating the duplicated iterate/try/catch loop over "/images/json".

🤖 Prompt for AI agents 
In osquery/tables/applications/posix/docker.cpp around line 1168, review and complete this code-review fix: osquery/tables/applications/posix/docker#osquery::tables::getImageHistoryAll duplicates an existing definition.
What the draft fix changed: getImageHistoryAll (formerly at line 1168) was removed and replaced by a call to the same new shared helper `getImageDetailsAll()`, passing the getImageHistory function as the per-image extractor. genImageHistory now calls `getImageDetailsAll(results, "image history", getImageHistory)` in its else branch instead of the removed getImageHistoryAll, eliminating the duplicated iterate/try/catch loop over "/images/json".
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

void getImageDetailsAll(
QueryData& results,
const std::string& error_label,
const std::function<void(const std::string&, QueryData&)>& extractor) {
pt::ptree tree;
Status s = dockerApi("/images/json", tree);
if (!s.ok()) {
Expand All @@ -1179,13 +1163,35 @@ void getImageHistoryAll(QueryData& results) {
if (boost::starts_with(id, "sha256:")) {
id.erase(0, 7);
}
getImageHistory(id, results);
extractor(id, results);
} catch (const pt::ptree_error& e) {
VLOG(1) << "Error getting docker image history: " << e.what();
VLOG(1) << "Error getting docker " << error_label << ": " << e.what();
}
}
}

/**
* @brief Entry point for docker_image_layers table.
*/
QueryData genImageLayers(QueryContext& context) {
QueryData results;
pt::ptree tree;
std::vector<std::string> layers;

if (context.constraints["id"].exists(
EQUALS)) { // get layers for specific image
for (const auto& id : context.constraints["id"].getAll(EQUALS)) {
if (!checkConstraintValue(id)) {
continue;
}
getImageLayers(id, results);
}
} else { // get layers for all images
getImageDetailsAll(results, "image details", getImageLayers);
}
return results;
}

/**
* @brief Entry point for docker_image_history table.
*/
Expand All @@ -1199,7 +1205,7 @@ QueryData genImageHistory(QueryContext& context) {
getImageHistory(id, results);
}
} else {
getImageHistoryAll(results);
getImageDetailsAll(results, "image history", getImageHistory);
}
return results;
}
Expand Down Expand Up @@ -1258,3 +1264,4 @@ QueryData genImageLabels(QueryContext& context) {
}
} // namespace tables
} // namespace osquery

Loading
Loading