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

DebugJournalistWrapper::jrnl_ used after being deleted #393

Closed
bradbell opened this issue Aug 23, 2020 · 9 comments · Fixed by #396
Closed

DebugJournalistWrapper::jrnl_ used after being deleted #393

bradbell opened this issue Aug 23, 2020 · 9 comments · Fixed by #396

Comments

@bradbell
Copy link

bradbell commented Aug 23, 2020

This bug shows up under the following conditions:

  1. The first IpoptApplciation in a program has been deleted.
  2. Ipopt is configured with --with-ipopt-verbosity
  3. One of the static dbg_verbosity flags (that gets used) is set to 1

The following bash script verbosity_bug.sh (changed estension .sh -> .txt so github would upload it) demonstrates the bug:
verbosity_bug.txt

@bradbell
Copy link
Author

bradbell commented Aug 23, 2020

I am attaching a patch file IpIpoptApplication.txt that uses a kludge to fix the problem (see Kludge comments in patch file). Use the following commands to run the patch:

cd src/Interface
patch src/Interfaces/IpIpoptApplication.cpp IpIpoptApplication.txt

IpIpoptApplication.txt

@svigerske
Copy link
Member

Thanks.
I thought I could just make the DebugJournalistWrapper::jrnl_ a smart-pointer, but trying to include IpSmartPtr.hpp in IpDebug.hpp led me straight into header hell.

So I went with your suggestion to have another (but smart) pointer to it in IpIpoptApplication. Pull request #396 should fix this.

@bradbell
Copy link
Author

The exact same thing (header hell) happened me.

Until someone figures out a better solution,
I think that we should put a comment in DebugJournalistWrapper::SetJournalist
saying that it is only called by the IpoptApplication constructor.

svigerske added a commit that referenced this issue Aug 24, 2020
- but accessible from IpoptApplication
- calling SetJournalist from somewhere else is dangerous, so forbid for
  now (see #393)
@svigerske
Copy link
Member

I can even allow only the IpoptApplication to call it by making this a private function.

@bradbell
Copy link
Author

Yes, in addition to the comment about who calls DebugJournalistWrapper::SetJournalist, it would be good to make the IpoptApplication class, or better yet its constructor, a friend of the DebugJournalistWrapper class and make the setting function private. I think that is what you are suggesting ?

@svigerske
Copy link
Member

yes, I think that's what 1de2097 is doing.

@bradbell
Copy link
Author

Yes. making the setting function private is a good idea.

I think the line
friend class IpoptApplication;
can be replaced by
friend IpoptApplication::IpoptApplication(bool, bool);
which would be more specific about which functions have access to the setting function.

@svigerske
Copy link
Member

Na, it's fine. I'm afraid that I would have to include IpIpoptApplication.hpp if I do that.

svigerske added a commit that referenced this issue Aug 24, 2020
- but accessible from IpoptApplication
- calling SetJournalist from somewhere else is dangerous, so forbid for
  now (see #393)
@svigerske
Copy link
Member

Fixed by #396.

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 a pull request may close this issue.

2 participants