Segfault in 4.0.0 - not-valid-enum - SerialTypes.cc:564

[initconf]

Seeing a segfault when I am trying to use config framework - I can reproduce it at will if I try with all the policies loaded. Trying to zero in to a reproducible script - but I couldn't reduce it further trying with a.zeek - so attaching the tar ball with all scripts.

To reproduce:

zeek ./scan-ng/scripts ./notice-report 

not-valid-enum-segfault.tar.gz

I suspect the crash has to do with :

│warning: Value 'Test::Blah' of source 'thread /YURT/feeds/zeek-FP/Notice::reports.zeek/Input::READER_CONFIG' is not a valid enum.

#0  zeek::IntrusivePtr<zeek::Type>::operator-> (this=0x20) at /home/bro/install/zeek-4.0.0/src/zeek/IntrusivePtr.h:152
#1  zeek::detail::CompositeHash::ComputeSingletonHash (this=0x562f5a0, v=0x0, type_check=true) at /home/bro/install/zeek-4.0.0/src/CompHash.cc:395
#2  0x0000000000722de5 in zeek::detail::CompositeHash::MakeHashKey (this=0x562f5a0, argv=..., type_check=true)
    at /home/bro/install/zeek-4.0.0/src/CompHash.cc:346
#3  0x000000000083edb5 in zeek::TableVal::MakeHashKey (this=0x56f5130, index=...) at /home/bro/install/zeek-4.0.0/src/Val.cc:2811
#4  zeek::TableVal::Assign (this=0x56f5130, index=..., new_val=..., broker_forward=<optimized out>, iterators_invalidated=0x0)
    at /home/bro/install/zeek-4.0.0/src/Val.cc:1554
#5  0x0000000000877583 in zeek::threading::Value::ValueToVal (source="thread /YURT/feeds/zeek-FP/Notice::reports.zeek/Input::READER_CONFIG",
    val=0x56c8600, have_error=@0x7fffffffe5e7: true) at /home/bro/install/zeek-4.0.0/src/threading/SerialTypes.cc:564
#6  0x0000000000870e65 in zeek::threading::Manager::SendEvent (this=<optimized out>, thread=0x433e800, name=..., num_vals=4, vals=0x56bcb40)
    at /home/bro/install/zeek-4.0.0/src/threading/Manager.cc:171
#7  0x0000000000873bf4 in zeek::threading::detail::SendEventMessage::Process (this=0x56cac90)
    at /home/bro/install/zeek-4.0.0/src/threading/MsgThread.cc:141
#8  0x0000000000872a98 in zeek::threading::MsgThread::Process (this=0x433e800) at /home/bro/install/zeek-4.0.0/src/threading/MsgThread.cc:477
#9  0x0000000000810930 in zeek::run_state::detail::run_loop () at /home/bro/install/zeek-4.0.0/src/RunState.cc:315
#10 0x00000000005f406e in main (argc=<optimized out>, argv=<optimized out>) at /home/bro/install/zeek-4.0.0/src/main.cc:59

[timwoj]

From asan:

warning in <no location>: multiple initializations for index (443/tcp)
warning in <no location>: multiple initializations for index (80/tcp)
warning in <no location>: multiple initializations for index (8080/tcp)
warning in <no location>: multiple initializations for index (8443/tcp)
warning: /feeds/BRO-feeds/subnets.csv-LATEST_BRO/Input::READER_ASCII: Init: cannot open /feeds/BRO-feeds/subnets.csv-LATEST_BRO
warning: /YURT/feeds/BRO-feeds/WIRED.blocknet/Input::READER_ASCII: Init: cannot open /YURT/feeds/BRO-feeds/WIRED.blocknet
warning: /YURT/feeds/zeek-FP/Scan::scan-config.zeek/Input::READER_CONFIG: Init: cannot open /YURT/feeds/zeek-FP/Scan::scan-config.zeek
warning: Value ' Scan::knockknockScan' for source 'thread ./Notice::reports.zeek/Input::READER_CONFIG' is not a valid enum.
AddressSanitizer:DEADLYSIGNAL
=================================================================
==5334==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000018 (pc 0x00010b3a1c4d bp 0x7ffee4883330 sp 0x7ffee4883330 T0)
==5334==The signal is caused by a READ memory access.
==5334==Hint: address points to the zero page.
    #0 0x10b3a1c4d in zeek::IntrusivePtr<zeek::Type>::operator->() const IntrusivePtr.h:152
    #1 0x10b5f13a8 in zeek::detail::CompositeHash::ComputeSingletonHash(zeek::Val const*, bool) const CompHash.cc:390
    #2 0x10b5f0db3 in zeek::detail::CompositeHash::MakeHashKey(zeek::Val const&, bool) const CompHash.cc:341
    #3 0x10b95c1d2 in zeek::TableVal::MakeHashKey(zeek::Val const&) const Val.cc:2760
    #4 0x10b957c4d in zeek::TableVal::Assign(zeek::IntrusivePtr<zeek::Val>, zeek::IntrusivePtr<zeek::Val>, bool, bool*) Val.cc:1552
    #5 0x10ba2ff53 in zeek::threading::Value::ValueToVal(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, zeek::threading::Value const*, bool&) SerialTypes.cc:564
    #6 0x10ba12c87 in zeek::threading::Manager::SendEvent(zeek::threading::MsgThread*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int, zeek::threading::Value**) const Manager.cc:171
    #7 0x10ba1e36c in zeek::threading::detail::SendEventMessage::Process() MsgThread.cc:141
    #8 0x10ba1d345 in zeek::threading::MsgThread::Process() MsgThread.cc:477
    #9 0x10ba1d42c in non-virtual thunk to zeek::threading::MsgThread::Process() MsgThread.cc
    #10 0x10b8b131a in zeek::run_state::detail::run_loop() RunState.cc:315
    #11 0x10c46231a in main main.cc:59
    #12 0x7fff20442620 in start+0x0 (libdyld.dylib:x86_64+0x15620)

==5334==Register values:
rax = 0x0000000000000003  rbx = 0x00007ffee4883440  rcx = 0x0000100000000003  rdx = 0x0000000000000000  
rdi = 0x0000000000000018  rsi = 0x0000603001b83050  rbp = 0x00007ffee4883330  rsp = 0x00007ffee4883330  
 r8 = 0x0000000000000000   r9 = 0x0000000000000000  r10 = 0xffffffffffffffff  r11 = 0x00000fffffffffff  
r12 = 0x00001fffdc910668  r13 = 0x00007ffee4883700  r14 = 0x00007ffee4883340  r15 = 0x0000000000000000  
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV IntrusivePtr.h:152 in zeek::IntrusivePtr<zeek::Type>::operator->() const
==5334==ABORTING
Abort trap: 6

[timwoj]

The problem is that we try to look up that value but get back a null pointer (in frame 5 above), and then try to use that null pointer as an index into a table (in frame 4). This shouldn't result in a segfault obviously. It would be enough to just warn and return false from TableVal::Assign. I'll open a PR for that in a minute.

[timwoj]

There's actually a second bug here aside from the crash. Looking at your scripts, this fails:

Notice::report_types Scan::LandMine, Scan::KnockKnockScan

but this succeeds:

Notice::report_types Scan::LandMine,Scan::KnockKnockScan

Note the space after the comma in the failing case is also reported in the trace I pasted in the comment above. The code should probably be stripping that space before trying to do the lookup. I'm not sure what the documentation says about comma-delimited lists like that.

[jsiwek]

The problem is that we try to look up that value but get back a null pointer (in frame 5 above), and then try to use that null pointer as an index into a table (in frame 4). This shouldn't result in a segfault obviously. It would be enough to just warn and return false from TableVal::Assign

Why shouldn't it segfault? Assigning to null table index isn't an operation that makes sense to attempt in the first place, so having TableVal::Assign() reject null indices can hide logical flaws in whoever is calling it.

So the flaws actually may be in the couple recursive usages of ValueToVal, which is claimed to return nullptr and set have_error parameter to true when an error occurs:

Think this should have checked if an error occurred, and if so, break the loop and return nullptr itself:

zeek/src/threading/SerialTypes.cc

Lines 561 to 565 in 7bcf6b4

	for ( int j = 0; j < val->val.set_val.size; j++ )
	{
	Val* assignval = ValueToVal(source, val->val.set_val.vals[j], have_error);
	t->Assign({AdoptRef{}, assignval}, nullptr);
	}

Same for this loop dealing with vectors:

zeek/src/threading/SerialTypes.cc

Lines 589 to 593 in 7bcf6b4

	for ( int j = 0; j < val->val.vector_val.size; j++ )
	{
	auto el = ValueToVal(source, val->val.vector_val.vals[j], have_error);
	v->Assign(j, {AdoptRef{}, el});
	}

[initconf]

Note the space after the comma in the failing case is also reported in the trace I pasted in the comment above.

Not entirely sure about space in comma delimited list is issue. I think the issue is a valid Notice::Type was Scan::KnockKnockScan ( with Upper case "K" - I ended up using lower case - thus config framework is unable to find)

However, I am not sure why I wasn't quite able to replicate with Test::blah ( a.zeek script)

[jsiwek]

Notice::report_types Scan::LandMine, Scan::KnockKnockScan

but this succeeds:

Notice::report_types Scan::LandMine,Scan::KnockKnockScan

Note the space after the comma in the failing case is also reported in the trace I pasted in the comment above. The code should probably be stripping that space before trying to do the lookup.

For an enum name like that, it could probably try to be helpful like that since identifiers can't contain space characters, but generally I'm not sure there should be an expectation of stripping whitespace between elements. Like if one has a set[string] and really does want leading whitespace in some element. (that example also raises another question that I didn't dig too far into: how someone can represent a string with a comma in it w/ an escaping mechanism or other way).

[jsiwek]

Not entirely sure about space in comma delimited list is issue. I think the issue is a valid Notice::Type was Scan::KnockKnockScan ( with Upper case "K" - I ended up using lower case - thus config framework is unable to find)

Yes, incorrect casing would be a problem, but it's also still sensitive to whitespace and you can test that.

[timwoj]

Yes, incorrect casing would be a problem, but it's also still sensitive to whitespace and you can test that.

Right, after fixing the casing issue it still failed because of the space. It's low-priority though since it's probably been like that forever.