Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix double free. #429

Merged
merged 1 commit into from
Feb 8, 2021
Merged

Conversation

Quipyowert2
Copy link
Contributor

@Quipyowert2 Quipyowert2 commented Feb 8, 2021

When a command starting with an asterisk was encountered, ModuleConfig
was called, which calls AddToModList which frees its argument
sometimes. Then __execute_function tries to free the same pointer
again. This commit fixes this by only freeing rline in AddToModList if
it points at xasprintf'd memory, as freeing the memory from xasprintf
won't invalidate expaction.

Fixes #425.

  • What does this PR do?
    Fixes a double free by conditionally freeing rline in AddToModList only if it doesn't point at tline anymore (by calling xasprintf).

The command causing the segfault was:
*FvwmIconMan*action Mouse 1 N sendcommand "Iconify off", sendcommand "FlipFocus", sendcommand "Raise"

  • Screenshots (if applicable)

  • PR acceptance criteria (reminder only, please delete once read)

    • Your commit message(s) are descriptive. See:

      https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

    • [https://raw.githubusercontent.com/fvwmorg/fvwm3/master/doc/README]Documentation updated (where appropriate)

    • Style guide followed (try and match the surrounding code where possible)

    • All tests are passing: although this is automatic, Codacy will often
      highlight additional considerations which will need to be addressed before
      the PR can be merged.

  • Issue number(s)

If this PR addresses any issues, please ensure the appropriate commit
message(s) contains:

Fixes #XXX

at the end of your commit message, where XXX should be replaced with the
relevant issue number.

If there is more than one issue fixed then use:

Fixes #XXX, fixes #YYY, fixes #ZZZ

@Quipyowert2 Quipyowert2 marked this pull request as draft February 8, 2021 01:04
@Quipyowert2 Quipyowert2 force-pushed the nm/fix-double-free branch 2 times, most recently from 06a069b to e70fc8e Compare February 8, 2021 02:22
@Quipyowert2 Quipyowert2 marked this pull request as ready for review February 8, 2021 02:23
Copy link
Member

@ThomasAdam ThomasAdam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Quipyowert2

While this is one approach, I really don't like littering the code with boolean logic as this just highlights a problem elsewhere to me -- namely that in this case, rline is a special case.

We can easily avoid using xasprintf() here if this stops this bool madness, a la strlcpy() for instance.

@Quipyowert2
Copy link
Contributor Author

Quipyowert2 commented Feb 8, 2021

I removed the boolean variable and added a check of whether rline and tline are the same pointer. The boolean variable wasn't actually needed in this case.

When a command starting with an asterisk was encountered, ModuleConfig
was called, which calls AddToModList which frees its argument
sometimes. Then __execute_function tries to free the same pointer
again. This commit fixes this by only freeing rline in AddToModList if
it points at xasprintf'd memory, as freeing the memory from xasprintf
won't invalidate expaction.

Fixes fvwmorg#425.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in __execute_function
2 participants