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

Interpreter exceptions cause memory leaks #166

Open
jsiwek opened this Issue Sep 17, 2018 · 0 comments

Comments

Projects
1 participant
@jsiwek
Copy link
Member

jsiwek commented Sep 17, 2018

Moved from https://bro-tracker.atlassian.net/browse/BIT-831

Created by Johanna Amann at 2012-06-08T18:51:11.569-0500:

The following bro script apparently triggers a memory-leak in the print statement.

event HTTP::log_http(rec: HTTP::Info)
{
	print fmt("%s %s", rec$md5, rec);
}

To reproduce run bro using 2009-M57-day11-18.trace. pprof output is attached.

Comment by Robin Sommer at 2012-06-11T10:21:02.731-0500:

This seems potentially significant. Anyone up for taking a look?

Comment by Jon Siwek at 2012-06-11T10:24:15.970-0500:

Replying to [robin|comment:1]:
This seems potentially significant. Anyone up for taking a look?

Yeah, I think I know what's going on, might have a patch shortly.

Comment by Jon Siwek at 2012-06-11T15:00:35.602-0500:

What's happening is the reference to rec$md5 does not first check whether that optional field is initialized (e.g. rec?$md5), so when it isn't, an internal InterpreterException gets thrown and there's no cleanup of heap allocations during the stack unwind. I'm not sure that catching, cleaning up allocated resources, and rethrowing that exception every place it can occur is going to be the greatest way to fix this. E.g. currently that exception only gets thrown from a couple implementations of UnaryExpr::Fold, but that's called from a couple implementations of Expr::Eval, which maybe used in ~70 places, then still the callers of those places also potentially need to handle the exception to do cleanup, etc.

Would it be better to wait and fix this through overhauling memory management to use smart pointers? That's planned, right? Any script that tries to reference a missing field should be corrected, anyway (these errors are logged in reporter.log).

Comment by Robin Sommer at 2012-06-18T13:58:43.261-0500:

Hmmm … That indeed sounds like a bigger problem. The exception is a relatively recent addition, before that Bro would just abort with an internal error for many runtime errors. So not much we can do right now.

Regarding smart pointers, yes, in principle, but that's a big task, with potential to introduce very subtle problems. So while it's on the roadmap, I'm actually reluctant to tackle it soon. Also, perhaps this can be taken core of by eventually compiling Bro scripts, which will remove lots of the relevant code anyway long-term. But that's all something for a longer discussion.

I'm leaving this ticket open, but I'm removing the milestone.

Comment by Johanna Amann at 2015-10-19T15:38:07.706-0500:

Closing - we will not be able to fix this anytime in the forseeable future because of the current internal architecture. Fixing this would be part of a major rewrite of the scripting functionality of Bro--and we will not forget about it; this is one of the well-known current gotchas of Bro.

Attachment https://bro-tracker.atlassian.net/secure/attachment/10217/bug.bro:

event HTTP::log_http(rec: HTTP::Info)
{
    print fmt("%s %s", rec$md5, rec);
}

@jsiwek jsiwek changed the title Interpreter exceptions cause memory leaks (was "Memory leak in print") Interpreter exceptions cause memory leaks Sep 17, 2018

@jsiwek jsiwek added this to Needs triage in Bug Triage via automation Sep 17, 2018

@jsiwek jsiwek moved this from Needs triage to High priority in Bug Triage Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment