voldyman / MarkMyWords

Markdown editor
MIT License
216 stars 19 forks source link

Fixes a small memory leak in Application.vala #91

Closed JMoerman closed 7 years ago

JMoerman commented 7 years ago

MarkMyWordsApp._command_line contains a memory leak issue. It shouldn't cause any problems, but I thought it would be best to fix it anyways.

I'll give a short description of what is going on.

In MarkMyWordsApp._command_line:

  1. string[] args = command_line.get_arguments (); args is set to an array of owned strings.
  2. unowned string[] tmp = args; tmp is an alias of args.
  3. context.parse (ref tmp); arguments in args are parsed and the references of these arguments are removed from args. (The strings themselves are not freed.) args contains the unparsed arguments followed by N*null, where N is the number of parsed arguments.
  4. End of _command_line: args is freed, only unparsed arguments are freed.

As the references to the parsed arguments are lost, they are never freed.

This pr fixes this in the following way:

  1. Create an array containing the references to the strings in args so the arrays contents are identical, but the arrays are not aliases.
  2. Make 'tmp' an alias of this array.

context.parse (ref tmp); will now remove the references to the parsed arguments in this new array, but args is not affected and all strings in returned by command_line.get_arguments () will now be freed.

I stumbled upon this problem a few days ago by reading some documentation on valadoc.org and realized that Go For It! contained this issue. I saw that you where the one who wrote the commandline bits of Go For It! so I figured that this small issue would also appear in your projects. (And probably in many more projects using OptionContext.parse (), because the documentation about this is pretty awful.)

voldyman commented 7 years ago

Thanks for the fix and the detailed description!

JMoerman commented 7 years ago

You're welcome! I did just notice that OptionContext.parse_strv does properly free the strings, so a simpeler fix was possible.