Copying members of a C struct in a loop - allocator leak

Help
, ,
1

Hello! This function takes a Linux username, gets a slice of GID’s, and then I use getgrouplist to turn those ID’s into human readable group name strings. I’m trying to copy the group name which is a C string from a C structure…

The problem that I’m running into is that using allocator.dupeZ in a loop doesn’t really allow me to ever free those strings without corrupting the underlying data of the ArrayList that returns from this function, or having a memory leak… What is the proper way of doing something like this?

Here is the code I wrote:

pub fn getgroups(allocator: Allocator, name: [*:0]const u8) ![][]const u8 {
    var groups_names = std.ArrayList([]const u8).init(allocator);
    defer groups_names.deinit();

    var ngroups: i32 = NGROUPS_MAX; // 1 << 8
    var groups_gids: [NGROUPS_MAX]gid_t = undefined;

    // get passwd struct
    if (std.c.getpwnam(name)) |pwd| {
        var gg: gid_t = 0;

        // this is ugly
        if (c.getgrouplist(pwd.name.?, pwd.gid, &groups_gids, &ngroups) == -1) return error.GroupListFail;
    } else {
        return error.NoPasswd;
    }

    if (ngroups != -1) {
        var i: usize = 0;
        while (i < ngroups) : (i += 1) {
            const g = c.getgrgid(groups_gids[i]);
            if (g == null) {
                continue;
            }

            // the root of the memory leak
            const gname = try allocator.dupeZ(u8, std.mem.span(g.*.gr_name));
            try groups_names.append(gname);
        }
    }

    return groups_names.toOwnedSlice();
}

Thank you!

2 
defer group_names.deinit()

will unconditionally deallocate the memory when the function exits, which is not really what you want since toOwnedSlice() gives responsibility of the memory to the caller to deallocate.

I haven’t looked too closely at the rest, but replacing that line with errdefer group_names.deinit() is probably what you’re looking for :slight_smile:

3

It leaks specifically with allocator.dupeZ for every single time the loop runs. I don’t think the array list is the problem, but copying these C strings out seems impossible.

If I free them, they either fault or I get garbage data in my ArrayList.

4

I don’t think that this is the cause, toOwnedSlice() sets the arraylist to empty, so the defer should be fine and can stay.

I think the cause of the memory leak is that @hexxy is using dupeZ which creates a sentinel terminated slice but than adding that to an arraylist of []const u8. You should either just use dupe to create a normal slice, or change the type to [:0]const u8 and the return type to [][:0]const u8.
So that the caller of the function has the type information to know that there is a sentinel that needs to be deallocated too.

Just as a side note the caller of getgroups needs to iterate over the slice and call free for every one of the strings (when it is time to deinit), and that will only work without a leak if the types match. So either use normal slices or sentinel terminated ones, but don’t pass sentinel terminated ones around as normal slices, because then the free call will not free the sentinel.

2 Likes
5

I tried both of these, but dupe/dupeZ still leaks every time the loop runs. I get the correct data and it only happens in Debug mode, maybe its not that much of a problem?

6

It is only detected in debug mode, in release you leak memory without detection.

Your code allocates data, when it is done using the data you need to free it, for example like this:

const groups = try getgroups(allocator, "something");
defer {
    for (groups) |g| allocator.free(g);
    allocator.free(groups);
}

// ...
2 Likes
7

Thank you! I see now that I was improperly freeing the ArrayList outside of the function, I thought it would have been adequate to just free the entire thing at once, but I see that I have to free each individual item. Have I just been doing that wrong this entire time?? Or is that just a result of the way that I allocated this particular list?

1 Like
8

I think it is because you allocated the individual items of this list.
Basically you have to do the thing you did to allocate it, also in reverse to free it.

Because of defers semantics of being applied in reverse order when you have multiple defer statements, that can often help with that.
But you have to carefully think through the logic, yes.

It might be that your other usage scenarios where of an easier type, that doesn’t have smaller individual allocations.

2 Likes
9

My mistake, you’re correct! Looking back on it, this feels like such an easy mistake to make. I wouldn’t be surprised to find this kind of leak somewhere in one of my projects…

2 Likes
10

True, I tend to try to avoid allocating as much as possible. Thank you again.

1 Like
11

I appreciate the help, nevertheless. Thanks.

1 Like
12

@hexxy
Since I assume you would delete all strings at once they all have the same livetime.

It makes sence to use ArenaAllocator to allocate all strings in such case.
It will:

  • put strings closer together making it potentially cache friendlier
  • would allow you to delete them all at once
  • ArenaAllocator generally allocates faster since it only needs to increment pointer
  • It’s a lot easier to not forget to delete stuff this way

You can check json parsing api for inspiration.

Code would look something like this

pub fn getgroups(allocator: Allocator, name: [*:0]const u8) !struct { [][]const u8, std.heap.ArenaAllocator } {
    var arena = std.heap.ArenaAllocator.init(allocator);
    errdefer arena.deinit();

    var groups_names = std.ArrayList([]const u8).init(arena.allocator());
    // no array defer since arena allocator will clean it up

    var ngroups: i32 = NGROUPS_MAX; // 1 << 8
    var groups_gids: [NGROUPS_MAX]gid_t = undefined;

    // get passwd struct
    if (std.c.getpwnam(name)) |pwd| {
        var gg: gid_t = 0;

        // this is ugly
        if (c.getgrouplist(pwd.name.?, pwd.gid, &groups_gids, &ngroups) == -1) return error.GroupListFail;
    } else {
        return error.NoPasswd;
    }

    if (ngroups != -1) {
        var i: usize = 0;
        while (i < ngroups) : (i += 1) {
            const g = c.getgrgid(groups_gids[i]);
            if (g == null) {
                continue;
            }

            // the root of the memory leak
            const gname = try arena.allocator().dupeZ(u8, std.mem.span(g.*.gr_name));
            try groups_names.append(gname);
        }
    }
    const slice = try groups_names.toOwnedSlice();
    return .{ slice, arena };
}

And at callsite

const groups, const arena = try getgroups(allocator, "something");
defer arena.deinit();

_ = groups; // use it here;
1 Like
13

Thanks! That seems really helpful. I’m going to give that a try and see how it goes. It would definitely be a lot more convenient and have less potential for accidents.

14

Also in original function defering ArrayList deiniting is not enough since you would need to delete all strings contained in it in case error happend. And arena solves this issue as well).

One way to caught this error without Arena is to write test for function using checkAllAllocationFailures. It will run the function multiple times and insert allocation failure at all allocation points. This way you can check what your error handling code works well. (I checked my and found a lot of issues)

2 Likes
15

I think using an arena is a good and practical idea, however personally I would create the arena at the call site and then pass the .allocator() as the allocator to the getgroups function.

Receiving an arena as a return value seems more complicated to me in this case.

3 Likes
16

I didn’t know about that testing feature, that seems really useful.

1 Like