Date: Wed, 19 Jun 2024 20:25:13 GMT From: Craig Leres <leres@FreeBSD.org> To: ports-committers@FreeBSD.org, dev-commits-ports-all@FreeBSD.org, dev-commits-ports-main@FreeBSD.org Subject: git: 20f7942a5e72 - main - security/zeek: Fix intermittent crash Message-ID: <202406192025.45JKPDK5084268@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by leres: URL: https://cgit.FreeBSD.org/ports/commit/?id=20f7942a5e720cd4ca1159950ca25ec853f121f6 commit 20f7942a5e720cd4ca1159950ca25ec853f121f6 Author: Craig Leres <leres@FreeBSD.org> AuthorDate: 2024-06-19 20:24:47 +0000 Commit: Craig Leres <leres@FreeBSD.org> CommitDate: 2024-06-19 20:24:47 +0000 security/zeek: Fix intermittent crash Also fix trace-summary python backtrace. Intermittent crash: https://github.com/zeek/zeek/commit/8c337bd7693a2002dcfe8a15b35dc92eb9e78de9 threading/MsgThread: Decouple IO source and thread A MsgThread acting as an IO source itself can result in the scenario where the threading manager's heartbeat timer deletes a terminated MsgThread instance, but at the same time this instance is in the list of ready IO sources as determined by the IO loop in the current iteration. trace-summary backtrace: https://github.com/zeek/pysubnettree/pull/38 Fix extension for stricter unicode validation This fixes our extension module for python/cpython#105375 which made unicode validation stricter. Reported by: Arne Welzel (crash), ogogon (backtrace) --- security/zeek/Makefile | 1 + ..._zeekctl_auxil_pysubnettree_SubnetTree__wrap.cc | 156 +++++++++++++++++++++ ...zeekctl_auxil_pysubnettree_include_SubnetTree.h | 11 ++ security/zeek/files/patch-src_threading_Manager.cc | 47 +++++++ security/zeek/files/patch-src_threading_Manager.h | 23 +++ .../zeek/files/patch-src_threading_MsgThread.cc | 146 +++++++++++++++++++ .../zeek/files/patch-src_threading_MsgThread.h | 62 ++++++++ 7 files changed, 446 insertions(+) diff --git a/security/zeek/Makefile b/security/zeek/Makefile index 32030bfb93be..791dd314aa14 100644 --- a/security/zeek/Makefile +++ b/security/zeek/Makefile @@ -1,5 +1,6 @@ PORTNAME= zeek DISTVERSION= 6.0.4 +PORTREVISION= 1 CATEGORIES= security MASTER_SITES= https://download.zeek.org/ diff --git a/security/zeek/files/patch-auxil_zeekctl_auxil_pysubnettree_SubnetTree__wrap.cc b/security/zeek/files/patch-auxil_zeekctl_auxil_pysubnettree_SubnetTree__wrap.cc new file mode 100644 index 000000000000..b8aeb6eacbed --- /dev/null +++ b/security/zeek/files/patch-auxil_zeekctl_auxil_pysubnettree_SubnetTree__wrap.cc @@ -0,0 +1,156 @@ +--- auxil/zeekctl/auxil/pysubnettree/SubnetTree_wrap.cc.orig 2024-05-16 17:25:57 UTC ++++ auxil/zeekctl/auxil/pysubnettree/SubnetTree_wrap.cc +@@ -1629,6 +1629,14 @@ SwigPyObject_repr(SwigPyObject *v, PyObject *args) + return repr; + } + ++/* We need a version taking two PyObject* parameters so it's a valid ++ * PyCFunction to use in swigobject_methods[]. */ ++SWIGRUNTIME PyObject * ++SwigPyObject_repr2(PyObject *v, PyObject *SWIGUNUSEDPARM(args)) ++{ ++ return SwigPyObject_repr((SwigPyObject*)v); ++} ++ + SWIGRUNTIME int + SwigPyObject_compare(SwigPyObject *v, SwigPyObject *w) + { +@@ -1741,11 +1749,7 @@ SWIGRUNTIME PyObject* + } + + SWIGRUNTIME PyObject* +-#ifdef METH_NOARGS +-SwigPyObject_next(PyObject* v) +-#else + SwigPyObject_next(PyObject* v, PyObject *SWIGUNUSEDPARM(args)) +-#endif + { + SwigPyObject *sobj = (SwigPyObject *) v; + if (sobj->next) { +@@ -1780,6 +1784,20 @@ SwigPyObject_acquire(PyObject* v, PyObject *SWIGUNUSED + return SWIG_Py_Void(); + } + ++#ifdef METH_NOARGS ++static PyObject* ++SwigPyObject_disown2(PyObject* v, PyObject *SWIGUNUSEDPARM(args)) ++{ ++ return SwigPyObject_disown(v); ++} ++ ++static PyObject* ++SwigPyObject_acquire2(PyObject* v, PyObject *SWIGUNUSEDPARM(args)) ++{ ++ return SwigPyObject_acquire(v); ++} ++#endif ++ + SWIGINTERN PyObject* + SwigPyObject_own(PyObject *v, PyObject *args) + { +@@ -1820,12 +1838,12 @@ swigobject_methods[] = { + #ifdef METH_O + static PyMethodDef + swigobject_methods[] = { +- {(char *)"disown", (PyCFunction)SwigPyObject_disown, METH_NOARGS, (char *)"releases ownership of the pointer"}, +- {(char *)"acquire", (PyCFunction)SwigPyObject_acquire, METH_NOARGS, (char *)"acquires ownership of the pointer"}, ++ {(char *)"disown", (PyCFunction)SwigPyObject_disown2, METH_NOARGS, (char *)"releases ownership of the pointer"}, ++ {(char *)"acquire", (PyCFunction)SwigPyObject_acquire2,METH_NOARGS, (char *)"acquires ownership of the pointer"}, + {(char *)"own", (PyCFunction)SwigPyObject_own, METH_VARARGS, (char *)"returns/sets ownership of the pointer"}, + {(char *)"append", (PyCFunction)SwigPyObject_append, METH_O, (char *)"appends another 'this' object"}, + {(char *)"next", (PyCFunction)SwigPyObject_next, METH_NOARGS, (char *)"returns the next 'this' object"}, +- {(char *)"__repr__",(PyCFunction)SwigPyObject_repr, METH_NOARGS, (char *)"returns object representation"}, ++ {(char *)"__repr__",(PyCFunction)SwigPyObject_repr2, METH_NOARGS, (char *)"returns object representation"}, + {0, 0, 0, 0} + }; + #else +@@ -1836,7 +1854,7 @@ swigobject_methods[] = { + {(char *)"own", (PyCFunction)SwigPyObject_own, METH_VARARGS, (char *)"returns/sets ownership of the pointer"}, + {(char *)"append", (PyCFunction)SwigPyObject_append, METH_VARARGS, (char *)"appends another 'this' object"}, + {(char *)"next", (PyCFunction)SwigPyObject_next, METH_VARARGS, (char *)"returns the next 'this' object"}, +- {(char *)"__repr__",(PyCFunction)SwigPyObject_repr, METH_VARARGS, (char *)"returns object representation"}, ++ {(char *)"__repr__",(PyCFunction)SwigPyObject_repr, METH_VARARGS, (char *)"returns object representation"}, + {0, 0, 0, 0} + }; + #endif +@@ -3457,7 +3475,7 @@ SWIGINTERN PyObject *SubnetTree___getitem__(SubnetTree + + PyObject* data = self->lookup(cidr, size); + if ( ! data ) { +- PyErr_SetString(PyExc_KeyError, cidr); ++ PyErr_SetObject(PyExc_KeyError, PyBytes_FromStringAndSize(cidr, size)); + return 0; + } + +@@ -4814,27 +4832,27 @@ static PyMethodDef SwigMethods[] = { + } + + static PyMethodDef SwigMethods[] = { +- { (char *)"SWIG_PyInstanceMethod_New", (PyCFunction)SWIG_PyInstanceMethod_New, METH_O, NULL}, +- { (char *)"inx_addr_sin_set", _wrap_inx_addr_sin_set, METH_VARARGS, NULL}, +- { (char *)"inx_addr_sin_get", _wrap_inx_addr_sin_get, METH_VARARGS, NULL}, +- { (char *)"inx_addr_sin6_set", _wrap_inx_addr_sin6_set, METH_VARARGS, NULL}, +- { (char *)"inx_addr_sin6_get", _wrap_inx_addr_sin6_get, METH_VARARGS, NULL}, +- { (char *)"new_inx_addr", _wrap_new_inx_addr, METH_VARARGS, NULL}, +- { (char *)"delete_inx_addr", _wrap_delete_inx_addr, METH_VARARGS, NULL}, +- { (char *)"inx_addr_swigregister", inx_addr_swigregister, METH_VARARGS, NULL}, +- { (char *)"new_SubnetTree", _wrap_new_SubnetTree, METH_VARARGS, NULL}, +- { (char *)"delete_SubnetTree", _wrap_delete_SubnetTree, METH_VARARGS, NULL}, +- { (char *)"SubnetTree_insert", _wrap_SubnetTree_insert, METH_VARARGS, NULL}, +- { (char *)"SubnetTree_remove", _wrap_SubnetTree_remove, METH_VARARGS, NULL}, +- { (char *)"SubnetTree_lookup", _wrap_SubnetTree_lookup, METH_VARARGS, NULL}, +- { (char *)"SubnetTree_prefixes", _wrap_SubnetTree_prefixes, METH_VARARGS, NULL}, +- { (char *)"SubnetTree_get_binary_lookup_mode", _wrap_SubnetTree_get_binary_lookup_mode, METH_VARARGS, NULL}, +- { (char *)"SubnetTree_set_binary_lookup_mode", _wrap_SubnetTree_set_binary_lookup_mode, METH_VARARGS, NULL}, +- { (char *)"SubnetTree___contains__", _wrap_SubnetTree___contains__, METH_VARARGS, NULL}, +- { (char *)"SubnetTree___getitem__", _wrap_SubnetTree___getitem__, METH_VARARGS, NULL}, +- { (char *)"SubnetTree___setitem__", _wrap_SubnetTree___setitem__, METH_VARARGS, NULL}, +- { (char *)"SubnetTree___delitem__", _wrap_SubnetTree___delitem__, METH_VARARGS, NULL}, +- { (char *)"SubnetTree_swigregister", SubnetTree_swigregister, METH_VARARGS, NULL}, ++ { "SWIG_PyInstanceMethod_New", SWIG_PyInstanceMethod_New, METH_O, NULL}, ++ { "inx_addr_sin_set", _wrap_inx_addr_sin_set, METH_VARARGS, NULL}, ++ { "inx_addr_sin_get", _wrap_inx_addr_sin_get, METH_VARARGS, NULL}, ++ { "inx_addr_sin6_set", _wrap_inx_addr_sin6_set, METH_VARARGS, NULL}, ++ { "inx_addr_sin6_get", _wrap_inx_addr_sin6_get, METH_VARARGS, NULL}, ++ { "new_inx_addr", _wrap_new_inx_addr, METH_VARARGS, NULL}, ++ { "delete_inx_addr", _wrap_delete_inx_addr, METH_VARARGS, NULL}, ++ { "inx_addr_swigregister", inx_addr_swigregister, METH_VARARGS, NULL}, ++ { "new_SubnetTree", _wrap_new_SubnetTree, METH_VARARGS, NULL}, ++ { "delete_SubnetTree", _wrap_delete_SubnetTree, METH_VARARGS, NULL}, ++ { "SubnetTree_insert", _wrap_SubnetTree_insert, METH_VARARGS, NULL}, ++ { "SubnetTree_remove", _wrap_SubnetTree_remove, METH_VARARGS, NULL}, ++ { "SubnetTree_lookup", _wrap_SubnetTree_lookup, METH_VARARGS, NULL}, ++ { "SubnetTree_prefixes", _wrap_SubnetTree_prefixes, METH_VARARGS, NULL}, ++ { "SubnetTree_get_binary_lookup_mode", _wrap_SubnetTree_get_binary_lookup_mode, METH_VARARGS, NULL}, ++ { "SubnetTree_set_binary_lookup_mode", _wrap_SubnetTree_set_binary_lookup_mode, METH_VARARGS, NULL}, ++ { "SubnetTree___contains__", _wrap_SubnetTree___contains__, METH_VARARGS, NULL}, ++ { "SubnetTree___getitem__", _wrap_SubnetTree___getitem__, METH_VARARGS, NULL}, ++ { "SubnetTree___setitem__", _wrap_SubnetTree___setitem__, METH_VARARGS, NULL}, ++ { "SubnetTree___delitem__", _wrap_SubnetTree___delitem__, METH_VARARGS, NULL}, ++ { "SubnetTree_swigregister", SubnetTree_swigregister, METH_VARARGS, NULL}, + { NULL, NULL, 0, NULL } + }; + +@@ -5399,9 +5417,9 @@ extern "C" { + char *ndoc = (char*)malloc(ldoc + lptr + 10); + if (ndoc) { + char *buff = ndoc; +- strncpy(buff, methods[i].ml_doc, ldoc); ++ memcpy(buff, methods[i].ml_doc, ldoc); + buff += ldoc; +- strncpy(buff, "swig_ptr: ", 10); ++ memcpy(buff, "swig_ptr: ", 10); + buff += 10; + SWIG_PackVoidPtr(buff, ptr, ty->name, lptr); + methods[i].ml_doc = ndoc; +@@ -5463,8 +5481,8 @@ SWIG_init(void) { + (char *)"this", &SwigPyBuiltin_ThisClosure, NULL, NULL, NULL + }; + static SwigPyGetSet thisown_getset_closure = { +- (PyCFunction) SwigPyObject_own, +- (PyCFunction) SwigPyObject_own ++ SwigPyObject_own, ++ SwigPyObject_own + }; + static PyGetSetDef thisown_getset_def = { + (char *)"thisown", SwigPyBuiltin_GetterClosure, SwigPyBuiltin_SetterClosure, NULL, &thisown_getset_closure diff --git a/security/zeek/files/patch-auxil_zeekctl_auxil_pysubnettree_include_SubnetTree.h b/security/zeek/files/patch-auxil_zeekctl_auxil_pysubnettree_include_SubnetTree.h new file mode 100644 index 000000000000..5b7a96b92efe --- /dev/null +++ b/security/zeek/files/patch-auxil_zeekctl_auxil_pysubnettree_include_SubnetTree.h @@ -0,0 +1,11 @@ +--- auxil/zeekctl/auxil/pysubnettree/include/SubnetTree.h.orig 2024-05-16 17:25:57 UTC ++++ auxil/zeekctl/auxil/pysubnettree/include/SubnetTree.h +@@ -147,7 +147,7 @@ class SubnetTree (public) + + PyObject* data = self->lookup(cidr, size); + if ( ! data ) { +- PyErr_SetString(PyExc_KeyError, cidr); ++ PyErr_SetObject(PyExc_KeyError, PyBytes_FromStringAndSize(cidr, size)); + return 0; + } + diff --git a/security/zeek/files/patch-src_threading_Manager.cc b/security/zeek/files/patch-src_threading_Manager.cc new file mode 100644 index 000000000000..9bafbd8c8b52 --- /dev/null +++ b/security/zeek/files/patch-src_threading_Manager.cc @@ -0,0 +1,47 @@ +--- src/threading/Manager.cc.orig 2024-05-16 17:25:52 UTC ++++ src/threading/Manager.cc +@@ -65,8 +65,12 @@ void Manager::Terminate() + delete *i; + } + ++ for ( auto* iosource : io_sources ) ++ delete iosource; ++ + all_threads.clear(); + msg_threads.clear(); ++ io_sources.clear(); + terminating = false; + } + +@@ -79,10 +83,11 @@ void Manager::AddThread(BasicThread* thread) + StartHeartbeatTimer(); + } + +-void Manager::AddMsgThread(MsgThread* thread) ++void Manager::AddMsgThread(MsgThread* thread, iosource::IOSource* iosource) + { + DBG_LOG(DBG_THREADING, "%s is a MsgThread ...", thread->Name()); + msg_threads.push_back(thread); ++ io_sources.push_back(iosource); + } + + void Manager::KillThreads() +@@ -129,6 +134,18 @@ void Manager::SendHeartbeats() + + t->Join(); + delete t; ++ } ++ ++ for ( auto it = io_sources.begin(); it != io_sources.end(); /**/ ) ++ { ++ auto* src = *it; ++ if ( ! src->IsOpen() ) ++ { ++ delete src; ++ it = io_sources.erase(it); ++ } ++ else ++ ++it; + } + } + diff --git a/security/zeek/files/patch-src_threading_Manager.h b/security/zeek/files/patch-src_threading_Manager.h new file mode 100644 index 000000000000..a75d05d8c79d --- /dev/null +++ b/security/zeek/files/patch-src_threading_Manager.h @@ -0,0 +1,23 @@ +--- src/threading/Manager.h.orig 2024-05-16 17:25:52 UTC ++++ src/threading/Manager.h +@@ -127,8 +127,9 @@ class Manager (protected) + * MsgThread constructor makes sure to do so. + * + * @param thread The thread. ++ * @param iosource The IO source of the thread. + */ +- void AddMsgThread(MsgThread* thread); ++ void AddMsgThread(MsgThread* thread, iosource::IOSource* iosource); + + void Flush(); + +@@ -148,6 +149,9 @@ class Manager (protected) + + using msg_thread_list = std::list<MsgThread*>; + msg_thread_list msg_threads; ++ ++ using io_source_list = std::list<iosource::IOSource*>; ++ io_source_list io_sources; + + bool did_process; // True if the last Process() found some work to do. + double next_beat; // Timestamp when the next heartbeat will be sent. diff --git a/security/zeek/files/patch-src_threading_MsgThread.cc b/security/zeek/files/patch-src_threading_MsgThread.cc new file mode 100644 index 000000000000..07a7bd863a46 --- /dev/null +++ b/security/zeek/files/patch-src_threading_MsgThread.cc @@ -0,0 +1,146 @@ +--- src/threading/MsgThread.cc.orig 2024-05-16 17:25:52 UTC ++++ src/threading/MsgThread.cc +@@ -213,7 +213,77 @@ bool ReporterMessage::Process() + + return true; + } ++// ++// The lifetime of the IO source is decoupled from ++// the thread. The thread may be terminated prior ++// to the IO source being properly unregistered and ++// forgotten by the IO manager. Specifically the ++// threading manager would delete an IO source which ++// the IO manager still believed to be ready. ++// ++// See issue #3682 for more details. ++class MsgThread_IOSource : public iosource::IOSource ++ { ++public: ++ explicit MsgThread_IOSource(MsgThread* thread) : thread(thread) ++ { ++ if ( ! iosource_mgr->RegisterFd(flare.FD(), this) ) ++ reporter->FatalError("Failed to register MsgThread fd with iosource_mgr"); + ++ SetClosed(false); ++ } ++ ++ ~MsgThread_IOSource() ++ { ++ if ( IsOpen() ) ++ { ++ if ( thread ) ++ reporter->Warning("Have thread %s set in MsgThread_IOSource", thread->Name()); ++ ++ if ( ! iosource_mgr->UnregisterFd(flare.FD(), this) ) ++ reporter->FatalError("Failed to unregister MsgThread fd from iosource_mgr"); ++ } ++ } ++ ++ void Process() override ++ { ++ flare.Extinguish(); ++ ++ if ( thread ) ++ thread->Process(); ++ else ++ { ++ // When there's no thread anymore, unregister ++ // this source from the IO manager and mark ++ // it as closed. The threading manager will then ++ // reap it during heartbeat processing or shutdown. ++ if ( ! iosource_mgr->UnregisterFd(flare.FD(), this) ) ++ reporter->FatalError("Failed to unregister MsgThread fd from iosource_mgr"); ++ ++ SetClosed(true); ++ } ++ } ++ ++ const char* Tag() override { return thread ? thread->Name() : "<MsgThread_IOSource orphan>"; } ++ ++ double GetNextTimeout() override { return -1; } ++ ++ void Fire() { flare.Fire(); }; ++ ++ // Fire the flare one more time so that ++ // the IO manager will call Process() and ++ // SetClosed(true). ++ void Close() ++ { ++ thread = nullptr; ++ flare.Fire(); ++ } ++ ++private: ++ MsgThread* thread = nullptr; ++ zeek::detail::Flare flare; ++ }; ++ + } // namespace detail + + ////// Methods. +@@ -232,19 +302,22 @@ MsgThread::MsgThread() : BasicThread(), queue_in(this, + child_finished = false; + child_sent_finish = false; + failed = false; +- thread_mgr->AddMsgThread(this); + +- if ( ! iosource_mgr->RegisterFd(flare.FD(), this) ) +- reporter->FatalError("Failed to register MsgThread fd with iosource_mgr"); +- +- SetClosed(false); ++ io_source = new detail::MsgThread_IOSource(this); ++ thread_mgr->AddMsgThread(this, io_source); + } + + MsgThread::~MsgThread() + { +- // Unregister this thread from the iosource manager so it doesn't wake +- // up the main poll anymore. +- iosource_mgr->UnregisterFd(flare.FD(), this); ++ // Unregister this thread from the IO source so we don't ++ // get Process() callbacks anymore. The IO source is ++ // freed by separately by the threading manager after its ++ // last Process() invocation. ++ if ( io_source ) ++ { ++ io_source->Close(); ++ io_source = nullptr; ++ } + } + + void MsgThread::OnSignalStop() +@@ -319,7 +392,14 @@ void MsgThread::OnKill() + + void MsgThread::OnKill() + { +- SetClosed(true); ++ // Ensure the IO source is closed and won't call Process() on this ++ // thread anymore. The thread got killed, so the threading manager will ++ // remove it forcefully soon. ++ if ( io_source ) ++ { ++ io_source->Close(); ++ io_source = nullptr; ++ } + + // Send a message to unblock the reader if its currently waiting for + // input. This is just an optimization to make it terminate more +@@ -432,7 +512,8 @@ void MsgThread::SendOut(BasicOutputMessage* msg, bool + + ++cnt_sent_out; + +- flare.Fire(); ++ if ( io_source ) ++ io_source->Fire(); + } + + void MsgThread::SendEvent(const char* name, const int num_vals, Value** vals) +@@ -514,8 +595,6 @@ void MsgThread::Process() + + void MsgThread::Process() + { +- flare.Extinguish(); +- + while ( HasOut() ) + { + Message* msg = RetrieveOut(); diff --git a/security/zeek/files/patch-src_threading_MsgThread.h b/security/zeek/files/patch-src_threading_MsgThread.h new file mode 100644 index 000000000000..1daab96b7e0c --- /dev/null +++ b/security/zeek/files/patch-src_threading_MsgThread.h @@ -0,0 +1,62 @@ +--- src/threading/MsgThread.h.orig 2024-05-16 17:25:52 UTC ++++ src/threading/MsgThread.h +@@ -30,6 +30,8 @@ class KillMeMessage; + class FinishedMessage; + class KillMeMessage; + ++class MsgThread_IOSource; ++ + } + + /** +@@ -43,7 +45,7 @@ class KillMeMessage; + * that happens, the thread stops accepting any new messages, finishes + * processes all remaining ones still in the queue, and then exits. + */ +-class MsgThread : public BasicThread, public iosource::IOSource ++class MsgThread : public BasicThread + { + public: + /** +@@ -213,19 +215,13 @@ class MsgThread : public BasicThread, public iosource: + */ + void GetStats(Stats* stats); + +- /** +- * Overridden from iosource::IOSource. +- */ +- void Process() override; +- const char* Tag() override { return Name(); } +- double GetNextTimeout() override { return -1; } +- + protected: + friend class Manager; + friend class detail::HeartbeatMessage; + friend class detail::FinishMessage; + friend class detail::FinishedMessage; + friend class detail::KillMeMessage; ++ friend class detail::MsgThread_IOSource; + + /** + * Pops a message sent by the child from the child-to-main queue. +@@ -291,6 +287,11 @@ class MsgThread : public BasicThread, public iosource: + */ + virtual const zeek::detail::Location* GetLocationInfo() const { return nullptr; } + ++ /** ++ * Process() forwarded by MsgThread_IOSource. ++ */ ++ void Process(); ++ + private: + /** + * Pops a message sent by the main thread from the main-to-chold +@@ -367,7 +368,7 @@ class MsgThread : public BasicThread, public iosource: + bool child_sent_finish; // Child thread asked to be finished. + bool failed; // Set to true when a command failed. + +- zeek::detail::Flare flare; ++ detail::MsgThread_IOSource* io_source = nullptr; // IO source registered with the IO manager. + }; + + /**
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202406192025.45JKPDK5084268>