Skip to content

feat: add rootfsPropagation support and improve error handling #105

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

Merged
ComixHe merged 1 commit into from
Aug 5, 2025
+67 −22
Merged

feat: add rootfsPropagation support and improve error handling #105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/linyaps_box/app.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,10 @@ try {
} },
options.subcommand_opt);
} catch (const std::exception &e) {
std::cerr << "Error: " << e.what() << std::endl;
LINYAPS_BOX_ERR() << "Error: " << e.what();
return -1;
} catch (...) {
std::cerr << "Error: unknown" << std::endl;
LINYAPS_BOX_ERR() << "unknown error";
return -1;
Comment on lines 82 to 84

@codereviewbot-ai codereviewbot-ai Bot Aug 5, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The error handling for unknown exceptions logs a very generic message, which might not be helpful for diagnosing issues. Consider logging more context-specific information if possible, or at least include a suggestion to check the logs for more details. This could help in troubleshooting and maintaining the application more effectively.

Suggested Improvement:

LINYAPS_BOX_ERR() << "Unknown error occurred. Please check the logs for more details.";

}

Expand Down
17 changes: 16 additions & 1 deletion src/linyaps_box/config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ parse_mount_options(const std::vector<std::string> &options)
continue;
}
if (auto it = propagation_flags_map.find(opt); it != propagation_flags_map.end()) {
propagation_flags &= it->second;
propagation_flags |= it->second;
continue;
}

Comment on lines 79 to 85

@codereviewbot-ai codereviewbot-ai Bot Aug 5, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The repeated pattern of checking for an option in a map and then setting a flag is a candidate for refactoring to reduce code duplication and improve maintainability. Consider creating a helper function that takes the option, the map, and a reference to the flag variable. This function would encapsulate the logic found in lines 79-85, making the code cleaner and easier to maintain.

Expand Down Expand Up @@ -262,6 +262,21 @@ linyaps_box::config::linux_t parse_linux(const nlohmann::json &obj,
linux.readonly_paths = std::move(readonly_paths);
}

if (auto rootfs_propagation = ptr / "rootfsPropagation"; obj.contains(rootfs_propagation)) {
auto val = obj[rootfs_propagation].get<std::string>();
if (val == "shared") {
linux.rootfs_propagation = MS_SHARED;
} else if (val == "slave") {
linux.rootfs_propagation = MS_SLAVE;
} else if (val == "private") {
linux.rootfs_propagation = MS_PRIVATE;
} else if (val == "unbindable") {
linux.rootfs_propagation = MS_UNBINDABLE;
} else {
throw std::runtime_error("unsupported rootfs propagation: " + val);
}
}

return linux;
}

Comment on lines 262 to 282

@codereviewbot-ai codereviewbot-ai Bot Aug 5, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The handling of rootfsPropagation using multiple if statements (lines 266-276) could be simplified and made more maintainable by using a map to associate string values with constants. This would reduce the complexity of the code and make it easier to add support for new propagation types in the future.

Expand Down
1 change: 1 addition & 0 deletions src/linyaps_box/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ struct config
std::optional<std::vector<id_mapping_t>> gid_mappings;
std::optional<std::vector<std::filesystem::path>> masked_paths;
std::optional<std::vector<std::filesystem::path>> readonly_paths;
Comment on lines 123 to 125

@codereviewbot-ai codereviewbot-ai Bot Aug 5, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The use of std::optional for std::vector types in lines 123-125 is unnecessary. A std::vector can be empty by default, which effectively represents 'no elements', making std::optional redundant. This not only simplifies the code but also avoids the overhead of checking whether the optional has a value.

Recommended Change:
Remove std::optional from gid_mappings, masked_paths, and readonly_paths, and handle them as regular vectors that can be empty if no elements are present.

unsigned int rootfs_propagation{ 0 };

@codereviewbot-ai codereviewbot-ai Bot Aug 5, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The rootfs_propagation field is initialized to zero without clear documentation or named constants, which can lead to confusion about the meaning of different values. Using named constants or an enum would improve the readability and maintainability of the code by making the mount propagation settings more explicit.

Recommended Change:
Define an enum or named constants that represent valid mount propagation settings and use these in place of the plain integer for rootfs_propagation.

};

std::optional<linux_t> linux;
Expand Down
67 changes: 48 additions & 19 deletions src/linyaps_box/container.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
{
bool deny_setgroups{ false };
bool mount_dev_from_host{ false };
unsigned int rootfs_propagation{ 0 };
};

container_data &get_private_data(const linyaps_box::container &c) noexcept
Expand Down Expand Up @@ -215,8 +216,16 @@
const_cast<char *const *>(c_args.data()), // NOLINT
const_cast<char *const *>(c_env.data())); // NOLINT

std::cerr << "execvp: " << strerror(errno) << " errno=" << errno << std::endl;
exit(1);
LINYAPS_BOX_ERR() << "execute hook " << [&bin, &c_args]() -> std::string {
std::stringstream stream;
stream << bin;
for (const auto &arg : c_args) {
stream << " " << arg;
}
return std::move(stream).str();
}() << " failed: "
<< strerror(errno);
_exit(EXIT_FAILURE);
}

int status = 0;
Expand Down Expand Up @@ -687,20 +696,31 @@

// we will pivot root later
LINYAPS_BOX_DEBUG() << "configure rootfs";
auto flags = config.linux->rootfs_propagation;
if ((flags & propagations_flag) == 0) {
flags = MS_PRIVATE | MS_REC;
}

// change the propagation type of rootfs mountpoint to configured type
// otherwise bind mount will inherit the propagation type of rootfs mountpoint
do_propagation_mount(linyaps_box::utils::open("/", O_PATH | O_CLOEXEC | O_DIRECTORY),
MS_REC | MS_PRIVATE);
flags);

// make sure the parent mount of rootfs is private
// make sure the parent mountpoint of new root is private
// pivot root will fail if it has shared propagation type
make_rootfs_private();

// pivot root will reset the propagation type of rootfs mountpoint
// we need to save the propagation type to make sure the parent mountpoint of new root is
// what we want
get_private_data(container).rootfs_propagation = flags;

LINYAPS_BOX_DEBUG() << "rebind container rootfs";

linyaps_box::config::mount_t mount;

Check warning on line 720 in src/linyaps_box/container.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Local variable 'mount' shadows outer variable
mount.source = root.current_path();
mount.destination = ".";
mount.flags = MS_BIND | MS_REC;
// mount.propagation_flags = MS_PRIVATE | MS_REC;
mount.flags = MS_BIND | MS_REC | MS_PRIVATE;
auto ret = do_mount(container, root, mount);
assert(!ret);

Expand Down Expand Up @@ -1278,7 +1298,7 @@
LINYAPS_BOX_DEBUG() << "Sync message sent";
}

void do_pivot_root(const std::filesystem::path &rootfs)
void do_pivot_root(const linyaps_box::container &container, const std::filesystem::path &rootfs)
{
LINYAPS_BOX_DEBUG() << "start pivot root";
LINYAPS_BOX_DEBUG() << linyaps_box::utils::inspect_fds();
Expand All @@ -1303,6 +1323,15 @@
throw std::system_error(errno, std::generic_category(), "pivot_root");
}

ret = fchdir(old_root.get());
if (ret < 0) {
throw std::system_error(errno, std::generic_category(), "fchdir");
}

// make sure that umount event couldn't propagate to host
do_propagation_mount(old_root, MS_REC | MS_PRIVATE);

// umount old root
ret = umount2(".", MNT_DETACH);
if (ret < 0) {
throw std::system_error(errno, std::generic_category(), "umount2");
Expand All @@ -1322,6 +1351,10 @@
if (ret < 0) {
throw std::system_error(errno, std::generic_category(), "chdir");
}

// restore the propagation type of rootfs mountpoint
do_propagation_mount(linyaps_box::utils::open("/", O_PATH | O_CLOEXEC | O_DIRECTORY),
get_private_data(container).rootfs_propagation);
}

void set_umask(const std::optional<mode_t> &mask)
Expand Down Expand Up @@ -1614,18 +1647,18 @@
wait_create_runtime_result(container, socket);
create_container_hooks(container, socket);
// TODO: selinux label/apparmor profile
do_pivot_root(rootfs);
do_pivot_root(container, rootfs);
set_umask(container.get_config().process.user.umask);
// processing all extensions before drop capabilities
processing_extensions(container);
set_capabilities(container, runtime_cap);
start_container_hooks(container, socket);
execute_process(process);
} catch (const std::exception &e) {
std::cerr << e.what() << std::endl;
LINYAPS_BOX_ERR() << "clone failed: " << e.what();
return -1;
} catch (...) {
std::cerr << "unknown error" << std::endl;
LINYAPS_BOX_ERR() << "clone failed: unknown error";
return -1;
}

Expand Down Expand Up @@ -1719,8 +1752,7 @@
return;
}

const auto code = errno;
std::cerr << "munmap: " << strerror(code) << std::endl;
LINYAPS_BOX_ERR() << "munmap child stack failed: " << strerror(errno);
assert(false);
}

Expand Down Expand Up @@ -1827,12 +1859,9 @@
}

c_args.push_back(nullptr);
auto ret = execvp(c_args[0], const_cast<char *const *>(c_args.data()));
if (ret < 0) {
exit(errno);
}

exit(0);
execvp(c_args[0], const_cast<char *const *>(c_args.data()));
LINYAPS_BOX_ERR() << "execute helper " << c_args[0] << " failed: " << strerror(errno);
_exit(EXIT_FAILURE);
}

int status = 0;
Expand Down Expand Up @@ -2214,7 +2243,7 @@
try {
execute_hook(hook);
} catch (const std::exception &e) {
std::cerr << "Error: " << e.what() << std::endl;
LINYAPS_BOX_ERR() << "execute poststop hook " << hook.path << " failed: " << e.what();
}
}
}
Expand Down