gvvaughan / lyaml

LibYAML binding for Lua.
gvvaughan.github.io/lyaml
Other
208 stars 34 forks source link

Fix memory leak in emitter #48

Closed jpflorijn closed 1 year ago

jpflorijn commented 1 year ago

For every map, sequence and scalar emitted, lyaml leaks six bytes, because the mapping style parameter is strdup()-ed but not freed.

This issue was first reported in #35.

The test code included in that report consistently showed leaks under at least lua5.1 and luajit 2.1.0-beta3; the following version under tighter GC settings makes the issue more visible though:

collectgarbage("setpause", 103)
collectgarbage("setstepmul", 1600)
local yaml=require("lyaml")
print(collectgarbage("count"),"kb")
for i = 1, 10 do
    for j=1,1000 do
        yaml.dump({a="b", c = {1, 2, 3}, d = {{3, 2, 1}, true, false}})
    end
    print(collectgarbage("count"),"kb")
    collectgarbage()
    print(collectgarbage("count"),"kb")
end

Fixed by freeing style analogously to encoding and type.

codecov[bot] commented 1 year ago

Codecov Report

Base: 97.14% // Head: 97.14% // No change to project coverage :thumbsup:

Coverage data is based on head (5c7ab75) compared to base (0701528). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #48 +/- ## ======================================= Coverage 97.14% 97.14% ======================================= Files 4 4 Lines 420 420 ======================================= Hits 408 408 Misses 12 12 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Gary+V.+Vaughan). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Gary+V.+Vaughan)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

gvvaughan commented 1 year ago

Thank you so much for making the time to debug this and submit a PR for the fix! Merged and released as v6.2.8.