Fixing a memory leak of xmlEntityPtr in librsvg | Hacker News Reader

Fixing a memory leak of xmlEntityPtr in librsvg

viruta.org

44 pointsJNRowe20 comments

Was curious if Chat GPT would be able to find the problem if I told it that there was a memory leak which it was able to find.

https://chatgpt.com/share/70af56e3-2fde-4dab-8b59-d05960dadd...

Just asking if there's a problem and asking Chat GPT to rank the error leads the memory leak error to be categorized as low criticality though.

https://chatgpt.com/share/68eb706a-05eb-44ca-8e2b-abc97c0422...

Pretty promising though that it was able to point out the problem even without pointing it out and it definitely caught the memory leak issue.

Claude 3.5 Sonnet was able to figure it out right away without pointing it out though.

Most languages have a rule of thumb when it comes to freeing resources; it is your responsibility to abide by them.

  Rust: drop
  C++: T::~T()
  C#: using + dispose
  Java: try-with + autocloseable, or finally
  Go: defer
  Haskell: bracket
  JavaScript: finally
  Python: finally or with + __exit__

Don't like it? Vote for linear types.

For a piece of code like this, it's too bad there is not a relatively easy conversion possible to an arena-based allocator. Because then the leak would not be relevant.

I wonder if it would be possible in Rust to do some magic at the Rust/C border in order to trick old C code into living inside an arena that is invisible to it?

Tangentially.. this is one of the things I don't like about a lot "newer" languages, like Rust and Zig. They seem to really love adding these single character sigils to the language that drastically change the meaning of a line of code or maybe an entire function.

As I get older my eyes strain more and all of this power packed into a single character really just puts me off. It seems like a strategy to emphasize writing code quickly rather than correctly, which is odd, given that this is opposite to the value proposition these languages purport to bring.

The ? is not really the issue here. Rust is similar to C++ in that it encourages implicit resource management using scopes, so if you have resources that have to be free'd, you have to implement Drop somewhere.

The code would have looked just as correct and would have been just as wrong with the "old" non-sigil `try!(...)` syntax.

> The ? is not really the issue here.

Based upon the way the code was written, it was, at least in the mind of the author. They forgot they could exit scope there. They clearly didn't _intend_ for that outcome, but ended up with it anyways, possibly out of habit, and possibly because a single impactful sigil like that is easy to miss in review.

I get that technically it didn't cause the memory leak.. but just look at the way that was written initially... it obviously led to it _within_ that particular structure.

> They forgot they could exit scope there.

Their mistake was writing resource management code that requires manually thinking about where they might exit scope.

Just curious, would writing it in a better language have prevented this?

This is a classic problem you can make in any language. If you were doing Java, and forgot to use `finally` for a resource, you'd have a leak due to an exception.

I do not know what metric you use to define "better"

`finally' is a word though, harder to forget than a single punctuation mark.

The dev did not "forget a single punctuation mark". It was there and it was correct. What was /not/ correct is that he did not implement any form of implicit resource freeing in an RAII-language. Doing resource management using scopes is a choice. It's not inherently better or worse than explicit resource management with something like Go's `defer`. But if that choice is made, as in Rust or C++, you have to ensure that the implicit resource management is correctly implemented (Drop in Rust, destructors in C++). Same as in Go, where you have to ensure that you call `defer` with the correct cleanup function or in C# where you have to use `using` on your disposables, etc.

Slightly tongue in cheek answer: it is easy to write this kind of code in C++ without having C involved, hence style guides will usually have a prominent guideline specifically against this type of code[1].

In Rust, I think you only really run into this issue when interacting with C (or otherwise engaging in unsafe code), so for normal Rust coding it doesn't need to be spelled out as a guideline. And the Rustonomicon[2], the go-to resource for unsafe Rust, isn't really written as a set of guidelines. At least from a brief search, I found it harder to find a Rust page that specifically says "don't do this".

1: E.g. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines..., the first guideline in the "resource management" section.

2: https://doc.rust-lang.org/nomicon/intro.html

fwiw, I have to agree that having "?" at the end of a call act (as I parse the explanation) as a "return" statement seems like a very odd and deceptive semantic to have (and as you say, harder to spot).

It seems to derive from the same reason why breaks and goto are generally frowned upon if not used judiciously - they lead to unexpected control flow.

It's not comparable to "goto" for the same reason that an early return isn't comparable to "goto". It's the furthest thing from "unexpected" control flow if you know what it does. And unlike "break" you can't accidentally mess it up. You HAVE to handle that error one way or another, and ? means "early return".

I can understand it not being intuitive to someone who has never seen it before, but that's purely a familiarity thing.

I really want to upvote you but you're at 13377 karma and I would hate to fuck that up!

Handling it with early return in this way still seems odd. I guess if everything is built to be done with automatic handling via scope, I can see why this way is OK. The way this would be done in my experience is just checking a return code, which feels like it's a bit more explicit if you want to early-return.

Still, lots of incorrect code has been written with that pattern as well due to incomplete cleanup, so it's not really fair to blame this syntax for it XD.

(I think most mechanisms that can jump far, including returns, need to be used carefully - nesting a return deep into a bunch of loops is just as bad)

You should see how much difference adding or removing a '*' can make in C when setting something to 0. I've written that bug before.

Or putting an N byte string into an N byte buffer (forgetting to leave room for the null terminator)

That's not a fair comparison though: you can't even see that single character! /s

Agree. Same applies to '!'. '!' is small compared to `== false` and tend to get missed. The IDE should highlight '!' and '?' in Rust.

On the contrary, IDE should fold 'if err != nil' in golang code to make meaningful code more visible.

Reply on news.ycombinator.com