merge topic/vern/case-insensitive-patterns

Description

This branch supports case-insensitive patterns/subpatterns, per discussion on the mailing list.

Environment

None

Activity

Show:
Jon Siwek
July 17, 2018, 7:28 AM

Awesome, thanks. It's merged now, though the I was curious about the change at:

https://github.com/bro/bro/commit/5ce3d1b899dc27adb853a19f07da35b7deeea9f0

And committed a follow-up change that would seem to make sense as it's fairly clear that the 'set_nfa' pointer can be null and 'nfa' non-null, which compare unequal and so result in the no-op of Unref()'ing a null pointer.

https://github.com/bro/bro/commit/c09fe427a8783830b2a208d33b909d875054930d

Even after that, I'm still thinking this method doesn't entirely clean up memory as it should, but also doesn't matter much because it's only used when parsing .sig files, which only ever happens once at init-time. Let me know if you or anyone else had contrary thoughts (about my follow-up patch, or the general-doubt).

Vern Paxson
July 17, 2018, 9:42 AM

Yeah, thanks for catching that (initial) case where set_nfa is still null.

Regarding incomplete cleanup, where do you see a potential for that? (Either way, I agree that it's no biggie since it only occurs when parsing .sig files.)

Jon Siwek
July 18, 2018, 12:44 AM

In the pattern compilation error code path:

https://github.com/bro/bro/blob/4c072409f04122bf96916a7aca2851f8fd5fbf6e/src/RE.cc#L166-L176

like when running bro -s test.sig -e 'print "hi"' with test.sig:

The reference count for "nfa" would have to be 1 to get cleaned up, but appears to be 20, so definitely lost when setting to null at:

https://github.com/bro/bro/blob/4c072409f04122bf96916a7aca2851f8fd5fbf6e/src/RE.cc#L174

Then on the more typical code path, I also didn't see where "nfa" gets reclaimed before setting it to point elsewhere:

https://github.com/bro/bro/blob/4c072409f04122bf96916a7aca2851f8fd5fbf6e/src/RE.cc#L183

Again, maybe not important to follow up on, but more curious to understand if the bug fix you did was in response to a real problem you encountered, if it was related to the case-insensitivity changes in a way I'm not seeing, or just some other explanation for it.

Vern Paxson
July 18, 2018, 2:55 AM

Hmmm is there any chance you're running on a branch that doesn't incorporate my leak fixes? make_alternate() used to leak, but it shouldn't anymore. If it doesn't leak, then I believe setting nfa=0 should be correct (not leak), and likewise it should be fine to assign to nfa outside of the while loop.

Regarding was the fix in response to a real problem, I think this specific fix was due to checking all of the uses of make_alternate() rather than directly observing a leak.

Jon Siwek
July 18, 2018, 6:37 AM

I've been looking at the newest master branch which includes your leak fixes. It now makes sense that you were just updating/checking usages of make_alternate(). I thought you could have been trying to get Specific_RE_Matcher::CompileSet() itself to not leak and suspicious that no one retained the pointer value contained in nfa before overwriting. Kind of a moot point since I mistook the intent.

Merged

Assignee

Jon Siwek

Reporter

Vern Paxson

Labels

None

External issue ID

None

Components

Fix versions

Priority

Normal