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

c18n: Duplicate string when adding compartment name #2328

Merged
merged 1 commit into from
Feb 18, 2025
Merged

Conversation

dpgao
Copy link
Contributor

@dpgao dpgao commented Feb 18, 2025

If the name argument is used, it needs to be duplicated to prevent it from being subsequently freed by others.

@dpgao dpgao requested a review from bsdjhb February 18, 2025 11:57
else
return (false);
}
else if (name != NULL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I do prefer the existing flow a bit, and a comment about why we strdup() is probably warranted. Maybe something like:

    /* Prefer DT_SONAME as the compartment base name if present. */
    if (obj->soname != NULL) {
        name = obj->soname;
    } else if (name != NULL) {
        /* If a name was provided, allocate a duplicate copy. */
        name = strdup(name);
    } else {
        /* Try to find a name if none provided. */
        ... (existing cases)
    }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might also be good to note in the commit message why supplied names are transient storage and require duplication. obj_main->path is not transient for example, so I guess it's the do_load_object case? If so, another approach might be to add strdup() explicitly in that call instead of doing it here? (I wonder if we could also just pass NULL when registering obj_main to avoid the strdup for that case?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realize my fix is flawed because now I cannot decide whether to call free(obj->name) in obj_free any more.

Note that object_add_name in do_load_object already unconditionally duplicates name, so we can piggyback on that copy and there's then no need to do anything extra in obj_free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also added an error message because this failure mode might not be obvious to the user.

This failure should be rare, but may happen if the user preloads an SONAME-less object using a file descriptor.

The name provided in the argument of c18n_add_obj is transient (it may
be passed by the user via dlopen) and thus cannot be directly stored
into an Obj_Entry. Instead, observe that this name would have already
been duplicated and added to obj->names, so we can just look there.
@dpgao dpgao force-pushed the c18n-procstat-fix-2 branch from abd44d4 to 384c94c Compare February 18, 2025 17:46
@dpgao dpgao requested a review from bsdjhb February 18, 2025 17:46
Copy link
Collaborator

@bsdjhb bsdjhb 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 this is fine. The old code was giving preference to the do_load_object name rather than some other name in &obj->names but it's not clear to me that that was really meaningful.

@dpgao dpgao merged commit 51e0489 into dev Feb 18, 2025
29 checks passed
@dpgao dpgao deleted the c18n-procstat-fix-2 branch February 18, 2025 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants