Skip to content
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.

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

Fix lxcfs read null #640

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Fix lxcfs read null #640

merged 1 commit into from
Sep 24, 2024

Conversation

DevonSchwartz
Copy link
Contributor

@DevonSchwartz DevonSchwartz commented May 1, 2024

We used the macros that were implemented for lxcfs_release() to fix the NULL path issue. This will replace the existing strcmp solution.

These macros have already been merged into the main branch to resolve the lxcfs_release() issue.

fix #635

@stgraber stgraber requested a review from mihalicyn May 1, 2024 22:15
@stgraber
Copy link
Member

stgraber commented May 1, 2024

@mihalicyn can you review this one please?

@mihalicyn
Copy link
Member

Hi @DevonSchwartz

Thanks for your PR! Please, can you explain how this is connected with #599 ? Why have you mentioned this issue?

@DevonSchwartz
Copy link
Contributor Author

I made a mistake linking that issue. I meant to link this issue instead. I will go ahead and fix that. Sorry for the confusion.

@mihalicyn
Copy link
Member

mihalicyn commented May 2, 2024

Next question is why are you change only lxcfs_read, but no touching other functions.

Also, please, can you explain the reason of this changes:
7117464
here, you are changing do_proc_read -> do_cg_read
1b8d493
then, do_cg_read -> do_proc_read

@DevonSchwartz
Copy link
Contributor Author

I change lxcfs_read because that was the only function specified. I did not want to change other functions that were not in the scope of the issue
We changed those because we were accidentally calling do_cg_read() for every case of LXCFS_TYPE, instead of the correct function. For example, when LXCFS_TYPE_PROC(type) was true, we should not have called do_cg_read. This issue came up when we compiled locally.

@mihalicyn
Copy link
Member

Ah, so, this PR is an attempt to fix #635?

@mihalicyn
Copy link
Member

We changed those because we were accidentally calling do_cg_read() for every case of LXCFS_TYPE, instead of the correct function. For example, when LXCFS_TYPE_PROC(type) was true, we should not have called do_cg_read. This issue came up when we compiled locally.

You need to squash all (3) commits into one then (https://docs.github.com/en/get-started/using-git/about-git-rebase). It is a good practice to have one commit per logical change. But in your case you have two commits (7117464 and 1b8d493) which do the opposite thing (first - introduces a wrong change and second reverts it). This is not something we want. ;-)

@mihalicyn
Copy link
Member

In general, I can't see how this change can fix issue in #635, because in there we have a pretty-pretty weird stack. It looks like something is terribly wrong with the dynamic symbol resolution. It is not the same bug as we had with lxcfs_release.

@DevonSchwartz
Copy link
Contributor Author

Just squashed the commits.

src/lxcfs.c Outdated

if (cgroup_is_enabled && strncmp(path, "/cgroup", 7) == 0) {
if (LXCFS_TYPE_CGROUP(type)) {
Copy link
Member

Choose a reason for hiding this comment

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

cgroup_is_enabled && LXCFS_TYPE_CGROUP(type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added change

Copy link
Member

@mihalicyn mihalicyn left a comment

Choose a reason for hiding this comment

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

small nit

@mihalicyn
Copy link
Member

Please, edit commit message:

 Combine last 3 commits into one commit that changes the macros

Changed the strcmp in lxcfs_read() to macros

Signed-off-by: Devon Schwartz <devon.s.schwartz@utexas.edu>

Fixed reads so that they have proper parameters

Signed-off-by: Devon Schwartz <devon.s.schwartz@utexas.edu>

Fixed semicolon and variable instantiation

Signed-off-by: Devon Schwartz <devon.s.schwartz@utexas.edu>

using git commit --amend. You need to have only one Signed-off-by tag and one commit desciption. You can take some inspiration about how we format git commit messages from git log.

@DevonSchwartz
Copy link
Contributor Author

Please, edit commit message:

 Combine last 3 commits into one commit that changes the macros

Changed the strcmp in lxcfs_read() to macros

Signed-off-by: Devon Schwartz <devon.s.schwartz@utexas.edu>

Fixed reads so that they have proper parameters

Signed-off-by: Devon Schwartz <devon.s.schwartz@utexas.edu>

Fixed semicolon and variable instantiation

Signed-off-by: Devon Schwartz <devon.s.schwartz@utexas.edu>

using git commit --amend. You need to have only one Signed-off-by tag and one commit desciption. You can take some inspiration about how we format git commit messages from git log.

small nit

Thank you! Just changed the previous commit message to one sign-off and fixed the cgroup_is_enabled issue.

@DevonSchwartz
Copy link
Contributor Author

DevonSchwartz commented May 5, 2024

I went ahead and added the checks to FUSE file system calls except open/opendir. Should we find a way to create a safe macro for the non-FUSE calls too?

Update: I just realized that those would may not have an equivalent macro because the macro relies on file type info.

@DevonSchwartz
Copy link
Contributor Author

DevonSchwartz commented May 6, 2024

Should I squash the latest commits to close this issue?

@stgraber
Copy link
Member

stgraber commented May 6, 2024

Yeah, a single clean commit would be good I think.

@DevonSchwartz
Copy link
Contributor Author

Should I resolve the change requested from above?

@DevonSchwartz
Copy link
Contributor Author

@mihalicyn Did I correctly address your desired changes? I think the merge is still blocked because of the review request on May 3.

@mihalicyn
Copy link
Member

Hey @DevonSchwartz

sorry for a long delay with the reply.

Yeah, now it looks good.

For some reason GitHub runners get stuck to test this one. I guess you need to do something like git commit --amend && git push -f to retrigger tests.

Signed-off-by: Devon Schwartz <devon.s.schwartz@utexas.edu>
@DevonSchwartz
Copy link
Contributor Author

Thanks @mihalicyn . Just made the push

@stgraber stgraber merged commit 68fa858 into lxc:main Sep 24, 2024
13 checks passed
@stgraber
Copy link
Member

Thanks!

mihalicyn added a commit to mihalicyn/lxcfs that referenced this pull request Oct 2, 2024
After lxc#640 was merged we've got the entire
procfs subtree unavailable.

Fixes: lxc#640
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

lxcfs: handle NULL in lxcfs_read (segfault at 0, code=killed, status=11/SEGV)
3 participants