5 ms·

Fixing a memory leak of xmlEntityPtr in librsvg

44 points by JNRowe 2y ago

akira2501 2y ago
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.
- filmor 2y ago
  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.
  - akira2501 2y ago
    > 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.
    - nemetroid 2y ago
      > They forgot they could exit scope there. Their mistake was writing resource management code that requires manually thinking about where they might exit scope.
      - oguz-ismail 2y ago
        Just curious, would writing it in a better language have prevented this?
        Dx5IQ 2y ago
        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"
        oguz-ismail 2y ago
        `finally' is a word though, harder to forget than a single punctuation mark.
        filmor 2y ago
        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.
        nemetroid 2y ago
        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#Rr-raii https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines..., the first guideline in the "resource management" section. 2: https://doc.rust-lang.org/nomicon/intro.html https://doc.rust-lang.org/nomicon/intro.html
- Panzer04 2y ago
  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.
  - dralley 2y ago
    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.
    - wizardforhire 2y ago
      I really want to upvote you but you're at 13377 karma and I would hate to fuck that up!
    - Panzer04 2y ago
      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)
- cobbal 2y ago
  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.
  - dralley 2y ago
    Or putting an N byte string into an N byte buffer (forgetting to leave room for the null terminator)
    - jeffrallen 2y ago
      That's not a fair comparison though: you can't even see that single character! /s
- deleted 2y ago
  [deleted]
- qouteall 2y ago
  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.
PiRho3141 2y ago
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-d05960dadd56 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-abc97c04227d 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.
jeffrallen 2y ago
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?
hfoiwufasdfw 2y ago
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.

44 points by JNRowe 2y ago

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.

filmor 2y ago
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.
- akira2501 2y ago
  > 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.
  - nemetroid 2y ago
    > They forgot they could exit scope there. Their mistake was writing resource management code that requires manually thinking about where they might exit scope.
    - oguz-ismail 2y ago
      Just curious, would writing it in a better language have prevented this?
      - Dx5IQ 2y ago
        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"
        oguz-ismail 2y ago
        `finally' is a word though, harder to forget than a single punctuation mark.
        filmor 2y ago
        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.
      - nemetroid 2y ago
        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#Rr-raii https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines..., the first guideline in the "resource management" section. 2: https://doc.rust-lang.org/nomicon/intro.html https://doc.rust-lang.org/nomicon/intro.html
Panzer04 2y ago
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.
- dralley 2y ago
  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.
  - wizardforhire 2y ago
    I really want to upvote you but you're at 13377 karma and I would hate to fuck that up!
  - Panzer04 2y ago
    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)
cobbal 2y ago
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.
- dralley 2y ago
  Or putting an N byte string into an N byte buffer (forgetting to leave room for the null terminator)
  - jeffrallen 2y ago
    That's not a fair comparison though: you can't even see that single character! /s
deleted 2y ago
[deleted]
qouteall 2y ago
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.

PiRho3141 2y ago

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-d05960dadd56 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-abc97c04227d 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.

jeffrallen 2y ago

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?

hfoiwufasdfw 2y ago

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.