merge topic/vern/case-insensitive-patterns


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




Jon Siwek
July 17, 2018, 7:28 AM

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

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.

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:

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:

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

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.



Jon Siwek


Vern Paxson



External issue ID



Fix versions