Memory management for interop with C libs

I’m looking to use libpcap to interact with my network interface but wondered whether I’m taking the right approach to the memory management side of things. I’ve put some comments around the code with some of my current understanding (may be completely wrong).

In short though:

  • I think it’s the case that when I call the C code, unlike Zig, it hides the allocator and handles it’s own memory internally, thus I have no direct access to the allocator as a whole. I presume I shouldn’t make assumptions as to what else they could be doing with their allocator and so I should call their pcap_freealldevs function.
  • Since I’m freeing the memory that the C lib was handling, I need to make copies of the strings before storing them in my list.
  • When I create the pcap_if pointer, I specify its type to be [*c]pcap.pcap_if_t. My understanding is that this creates a C pointer which points to a struct of type pcap_if_t.
  • I only need to errdefer the deinit of the list since under normal circumstances the toOwnedSlice will do that for me. Is it ok for me to call this as a standard defer instead and what’s best practise in this situation?

If anyone wants to tear my code apart and show me a more Zig like approach, I’d really appreciate it.

Note: I’m using the page allocator here just while I was testing and probably shoul switch to a different one

const std = @import("std");
const pcap = @cImport(@cInclude("pcap.h"));

pub fn main() !void {}

const NetworkError = error{DevicesNotAvailable};

// TODO: Need to figure out how to specify network error and memory error
fn getListOfNetworkDevices(allocator: std.mem.Allocator) (NetworkError || error{OutOfMemory})![][]u8 {

    // Create array list using passed in allocator
    // This ensures the caller is in control of memory
    var networkList = std.ArrayList([]u8).init(allocator);

    // We errdefer here since deinit is not necessary if copy to owned slice
    errdefer networkList.deinit();

    // Setup a slice of pointers for structs of type pcap_if_t
    // TODO: Not sure if we should be specifying [*c] pointer type explicitly.
    var pcap_if: [*c]pcap.pcap_if_t = undefined;

    // Create a buffer for the error, if there is one. The error will
    // be populated if the return value is not 0.
    var errBuff: [pcap.PCAP_ERRBUF_SIZE]u8 = undefined;

    // Find all devices this process has access to. The return value is
    // 0 even if no devices are found. If no devices are found the pcap_if
    // will be NULL
    const findDevsRetVal = pcap.pcap_findalldevs(&pcap_if, &errBuff);

    // Defers the free of memory handled by the find all devs
    // Note: Memry in this case is handled internally by C libs so isn't zig like
    defer pcap.pcap_freealldevs(pcap_if);

    if (findDevsRetVal == 0) {
        if (pcap_if != null) {
            var curIf = pcap_if;
            while (curIf != null) {
                // We make a copy of the string since the c allocator owns the string
                // and we need to call feealldevs. Maybe it would be safe for us to
                // not call pcap_freealldevs and thus keep hold of the memory ourselves
                // but I'm not sure if there's other memory being held unecessarily??
                // Maybe a misunderstanding on my part.
                const myStr = try allocator.dupe(u8, std.mem.span(curIf.*.name));
                try networkList.append(myStr);
                curIf = curIf.*.next;
            }
        }
        return networkList.toOwnedSlice();
    } else {
        return NetworkError.DevicesNotAvailable;
    }
}

test "simple test" {
    const t = try getListOfNetworkDevices(std.heap.page_allocator);

    std.debug.print("Doing something", .{});
    for (t) |elem| {
        std.debug.print("ELEM: {s}\n", .{elem});
    }
}

Generally your code looks good to me.

I got a few things to point out, but none that you necessarily need to change here.

Copying the strings from C into a custom allocator and freeing it either with the library API or the c_allocator if no API is provided are definitely the way to go.

// TODO: Not sure if we should be specifying [*c] pointer type explicitly.

The [*c] types signifies to the compiler and the reader that you don’t know whether it’s a pointer to single item, a pointer to a list of items or a pointer to a null-terminated list of items.
So generally I would avoid using C pointers in zig code, and instead use * [*] or [*:0].
Now in this particular case that would require adding a pointer cast, which isn’t great either. So it’s fine here, I guess.

I only need to errdefer the deinit of the list since under normal circumstances the toOwnedSlice will do that for me. Is it ok for me to call this as a standard defer instead and what’s best practise in this situation?

It’s safe to call defer deinit here. From the documentation:

“Its capacity is cleared, making deinit() safe but unnecessary to call.”

I would prefer defer in cases like this because it makes it safer for future edits. For example if you add another special-case return in the future, then the arraylist will be cleared automatically.

But in the end it’s up to you to decide here. Choose the one that best fits what you want to express.

2 Likes

Brilliant, thanks. I had tried defining the pointer as just a * but the compiler complained saying it was expecting a [*c][*c] and I was passing it a **. Didn’t realise that I’d need a pointer cast, so makes sense it didn’t work as I had expected.

I think I understand the way Zig wants us to handle memory when purely within the Zig ecosystem, but when combined with C, I must admit, it makes my brain hurt. I’m sure it’ll come in time though. Thanks for the pointers (no pun intended)