mirror/freebsd-ports
1
0
Fork 0
mirror of https://git.FreeBSD.org/ports.git synced 2025-02-07 11:49:40 +00:00
Code Issues Releases Activity

security/zeek: Fix intermittent crash

Browse Source 
Also fix trace-summary python backtrace.

Intermittent crash:

    8c337bd769

    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)
This commit is contained in:
Craig Leres 2024-06-19 13:24:47 -07:00
parent 72d04ff33d
commit 20f7942a5e
7 changed files with 446 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
security/zeek/Makefile
View File

@ -1,5 +1,6 @@
PORTNAME= zeek
DISTVERSION= 6.0.4
PORTREVISION= 1
CATEGORIES= security
MASTER_SITES= https://download.zeek.org/

156
security/zeek/files/patch-auxil_zeekctl_auxil_pysubnettree_SubnetTree__wrap.cc Normal file
View File

@ -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

11
security/zeek/files/patch-auxil_zeekctl_auxil_pysubnettree_include_SubnetTree.h Normal file
View File

@ -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;
}

47
security/zeek/files/patch-src_threading_Manager.cc Normal file
View File

@ -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;
}
}

23
security/zeek/files/patch-src_threading_Manager.h Normal file
View File

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

146
security/zeek/files/patch-src_threading_MsgThread.cc Normal file
View File

@ -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();

62
security/zeek/files/patch-src_threading_MsgThread.h Normal file
View File

@ -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.
};
/**