#Help with memory leak/segmentation fault

1 messages · Page 1 of 1 (latest)

tawdry dagger
#

I am using Mocha to parse a config. On deinit, the GBA detects a memory leak. This is because I didn't run the .deinit() function on the config variable, however when used as shown in the following snippet, it causes a dangling pointer I think and segfaults. How can I free config without this happening? In this snippet: https://zigbin.io/99e49e I tried to allocate the value, but no luck.

mortal knot
#

that alloc.create looks unnecessary

#

and you're also not freeing it

#

you can probably delete lines 10 and 11 and just return deserialised

#

you're also not deferring the config.deinit(alloc)

tawdry dagger
mortal knot
#

all it's doing is copying a pointer for no reason

tawdry dagger
# 
pub fn setConfigFromPath(alloc: std.mem.Allocator, path: []const u8) !Self {
    const file = try std.fs.cwd().readFileAlloc(alloc, path, 512);
    defer alloc.free(file);

    var config = try mocha.Parser.parse(alloc, file);
    defer config.deinit(alloc);

    return try config.deserialize(Self, alloc);
}
#

This segfaults as well when I access it

mortal knot
#

what does config.deserialize do

#

if it is returning a pointer into the config you will be getting undefined behaviour

tawdry dagger
mortal knot
#

and what are the fields of Self

tawdry dagger
#

Currently just channels: []const List.Item

# 
pub const Item = struct {
    selected: bool = false,
    title: []const u8,
    description: []const u8,
};
mortal knot
#

well your problem is probably that config.deserialize is returning pointers to inside of the config, which you are freeing

hard heart
#

I concur.

tawdry dagger
#

Yeah, how could I avoid that? Do I have to modify Mocha?

#

I already have a slight fork, so maybe something I did broke it too...

mortal knot
#

you could duplicate all the fields out or don't free config until you are completely done

hard heart
#

Not sure if config needs file to stick around, but assuming so, the simplest is probably to just not free the file contents, and return that as well.

tawdry dagger
#

It doesn't need file to stick around

hard heart
#

Seems odd for it to duplicate it. 🤔
I'd double check that.
Either way though, keep whichever is needed around (file or config), or copy out just the bits you actually need to keep alive.

#

i.e: If Self has some string fields that you need to keep alive, then you can use something like this to keep it alive:

self.that_field = try some_other_allocator.dupe(u8, self.that_field);
mortal knot
#

my favourite parsing libraries are the ones that provide tokeniser APIs, then you can control exactly what is allocated

tawdry dagger
#

Is there something like dupe for single items?

mortal knot
#

for single items you can probably just not allocate them and return them directly

#

if you're doing the copy approach, you'll need to allocate a slice of List.Item, and allocate duplicates of title and description

#

there shouldn't be any single item allocations there

#

but if you really needed it, you would probably .create and set it manually

tawdry dagger
#

I think the issue has to do with string fields being stored in config

tawdry dagger
# 
.string => |string| Value{ .string = try allocator.dupeZ(u8, string) },

Okay, so if inside Mocha I duplicate the string it will work, but I get the memory leak detected on exit. So now I have to figure out how to free this...

#

I tried string.*, but that gives error: index syntax required for slice type '[:0]const u8'

mortal knot
#

you need to pass the slice into the allocator

hard heart
# tawdry dagger ```js .string => |string| Value{ .string = try allocator.dupeZ(u8, string) }, ``...

I think this is the sane approach, but not here; I would just do this:

fn clone(item: Item, ally: Allocator) !Item {
    const buf = try ally.alloc(u8, item.title.len + item.description.len);
    const title = buf[0..item.title.len];
    @memcpy(title, item.title);
    const desc = buf[item.title.len..];
    @memcpy(desc, item.description);
    return .{
        .selected = item.selected,
        .title = title,
        .description = desc,
    };
}

Then you can just do:

const self = try config.deserialize(Self, alloc);
return self.clone();
tawdry dagger
#

instead of hacking something on top

#

which is what I was trying to do with the snippet above, but I am unsure how to do it without an allocator

hard heart
# tawdry dagger I think I would prefer to fix it in the library itself?

Depends what you mean by 'fix' I suppose.

This arrangement is a pretty common one in parsers, since most of the time you'd use an arena, and destroy the whole lot at once - save for the one piece that you actually want to keep around.

I'm also not sure how practical it is to parse without an allocator, unless you're doing a streaming parser that hands you each piece as you ask for more or something.
Even in that case, you'd have to duplicate the parts you wanted to keep around, so I'm not sure that would really help you here.

tawdry dagger
#

I considered using an arena allocator, might as well just do that