merge topic/vern/case-insensitive-patterns

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).

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.)

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.

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.

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.