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 endless loop in get_user_groups #1146

Merged
merged 1 commit into from
Nov 17, 2023
Merged

Fix endless loop in get_user_groups #1146

merged 1 commit into from
Nov 17, 2023

Conversation

GuillaumeGomez
Copy link
Owner

Fixes #1145.

Can you confirm it fixes the issue for you @aw-cf please?

Copy link

@aw-cf aw-cf left a comment

Choose a reason for hiding this comment

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

I would suggest to revert those changes again and fix this by just adding groups.set_len(nb_groups as _); before calling reserve.

Here is what the full function would look like:

pub(crate) unsafe fn get_user_groups(
    name: *const libc::c_char,
    group_id: libc::gid_t,
) -> Vec<Group> {
    let mut buffer = Vec::with_capacity(2048);
    let mut groups = Vec::with_capacity(256);

    loop {
        let mut nb_groups = groups.capacity();
        if getgrouplist(
            name,
            group_id as _,
            groups.as_mut_ptr(),
            &mut nb_groups as *mut _ as *mut _,
        ) == -1
        {
            // Ensure the length matches the number of returned groups.
            // Needs to be updated for `Vec::reserve` to actually add additional capacity.
            groups.set_len(nb_groups as _);
            groups.reserve(256);
            continue;
        }
        groups.set_len(nb_groups as _);
        return groups
            .iter()
            .filter_map(|group_id| {
                let name = get_group_name(*group_id as _, &mut buffer)?;
                Some(Group {
                    name,
                    id: Gid(*group_id as _),
                })
            })
            .collect();
    }
}

if getgrouplist(
name,
group_id as _,
groups.as_mut_ptr(),
&mut nb_groups as *mut _ as *mut _,
) == -1
{
groups.reserve(256);
groups.reserve(REALLOC_SIZE);
Copy link

Choose a reason for hiding this comment

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

reserve is not doing anything here because it will only act when additional > .capacity() - .len().
So the key change that is missing here is to modify the length via groups.set_len(nb_groups as _) before calling reserve() .

I recommend adding some logging before calling reserve and setting REALLOC_SIZE = 1 to make the issue more clear.

            println!("before reserve: capacity: {} length: {}", groups.capacity(), groups.len());
            groups.reserve(1);
            println!("after reserve: capacity: {} length: {}", groups.capacity(), groups.len());

The current change is causing memory access violation because nb_groups gets larger than the capacity of groups.

@GuillaumeGomez
Copy link
Owner Author

Oh I see. I completely misunderstood the issue... Thanks for the explanations. Don't you prefer to send a PR? At this point you already did all the work so you'd get ownership of the commit as well.

@aw-cf
Copy link

aw-cf commented Nov 17, 2023

No worries and please feel free to push the changes yourself given you have the PR up already.

Co-authored-by: aw-cf <>
@GuillaumeGomez
Copy link
Owner Author

I copied your code and I added you as co-author (in an incomplete way as I don't have your email). If you want me to add the email, don't hesitate to send it to me, otherwise if you don't want, I'll just merge as is.

Thanks a lot again!

Copy link

@aw-cf aw-cf left a comment

Choose a reason for hiding this comment

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

Thank you for getting this fixed right away!

@GuillaumeGomez
Copy link
Owner Author

It'll be part of the next major release though. I'm hoping to have it finished in the next weeks.

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.

Endless loop in get_user_groups
2 participants