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

fixed a memory leak. #38

Closed
wants to merge 2 commits into from
Closed

fixed a memory leak. #38

wants to merge 2 commits into from

Conversation

Kray-G
Copy link

@Kray-G Kray-G commented Mar 19, 2021

I found this code has a memory leak.
This is a simple fix for that.
Please check it.

A memory will be allocated every time whenever calling soliter_next and it will be never freed.
This means a memory will be leaked each iteration,
so amount of leaks are increased in proportion to the number of iterations.

@github-actions
Copy link

Hi there, thank you for your contribution!

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further activity occurs.

If you would like this pull request to remain open please:

  1. Rebase and verify the changes still work
  2. Leave a comment with the current status

NOTE: If this pull request was closed prematurely, please leave a comment.

Thanks!

@github-actions github-actions bot added the stale [bot] marked as stale due to inactivity label Mar 23, 2022
@github-actions
Copy link

Hi there, thank you for your contribution!

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further activity occurs.

If you would like this pull request to remain open please:

  1. Rebase and verify the changes still work
  2. Leave a comment with the current status

NOTE: If this pull request was closed prematurely, please leave a comment.

Thanks!

@Kray-G
Copy link
Author

Kray-G commented Mar 23, 2022

I think this is a bug. It would be really recommended to fix it.

@github-actions github-actions bot added stale::recovered [bot] recovered after being marked as stale and removed stale [bot] marked as stale due to inactivity labels Mar 24, 2022
@jezdez jezdez requested a review from a team October 14, 2022 19:04
@dholth
Copy link
Contributor

dholth commented Oct 14, 2022

I'm looking at the picosat source code, which uses a static signed char * sol; and allocates it the first time this function is called.

static void
blocksol (PicoSAT * picosat)
{
  int max_idx = picosat_variables (picosat), i;

  if (!sol)
    {
      sol = malloc (max_idx + 1);
      memset (sol, 0, max_idx + 1);
    }

  for (i = 1; i <= max_idx; i++)
    sol[i] = (picosat_deref (picosat, i) > 0) ? 1 : -1;

  for (i = 1; i <= max_idx; i++)
    picosat_add (picosat, (sol[i] < 0) ? i : -i);

  picosat_add (picosat, 0);
}

I think @Kray-G is probably correct. When we update *mem the caller doesn't see it, because it passed the memory address by value. I guess C warnings do not point out this problem for us? Will need to take the time to recompile it and see how it goes.

@dholth
Copy link
Contributor

dholth commented Oct 14, 2022

Thank you very much for your submission. Can you take a look at the shorter fix at https://github.com/conda/pycosat/pull/54/files to see if it would solve the same problem, with a bit less pointer indirection?

@jezdez
Copy link
Member

jezdez commented Oct 24, 2022

Superseded by #54

@jezdez jezdez closed this Oct 24, 2022
@github-actions github-actions bot added the locked [bot] locked due to inactivity label Oct 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity stale::recovered [bot] recovered after being marked as stale
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants