From dc25f57b37d7c45739cc50fa64e42b80179436e8 Mon Sep 17 00:00:00 2001 From: Darren Garnier Date: Tue, 21 Jul 2026 18:48:36 +1200 Subject: [PATCH 1/4] Tests: GetMany queries that succeed with a nil result (issue #3070) A GetMany batch containing a query that evaluates successfully to a nil/missing result crashes the mdsip worker: getManyObj() stores the NULL Data* under the 'value' key, and Dictionary::setItem dereferences it (data->incRefCount()). Add MdsGetManyTest covering: - Dictionary holding a missing (NULL) value: setItem, getItem, len, and replacing missing<->data - the serialize/deserialize round trip of a dictionary with a missing value (the transport format of the GetManyExecute reply) - GetManyExecute() end to end with a query mirroring the field case, DATA(BEGIN_OF(BUILD_RANGE(*, *, 5E-6))), asserting the answer is { 'value': missing } exactly as a plain get() of the same expression would return The test crashes without the corresponding fix to Dictionary::setItem / getItem in include/mdsobjects.h. Co-Authored-By: Claude Fable 5 --- mdsobjects/cpp/testing/CMakeLists.txt | 1 + mdsobjects/cpp/testing/MdsGetManyTest.cpp | 137 ++++++++++++++++++++++ 2 files changed, 138 insertions(+) create mode 100644 mdsobjects/cpp/testing/MdsGetManyTest.cpp diff --git a/mdsobjects/cpp/testing/CMakeLists.txt b/mdsobjects/cpp/testing/CMakeLists.txt index f3b5286158..1f661d851c 100644 --- a/mdsobjects/cpp/testing/CMakeLists.txt +++ b/mdsobjects/cpp/testing/CMakeLists.txt @@ -59,6 +59,7 @@ set(_test_source_list MdsEventSuppression.cpp MdsEventTest.cpp MdsConnectionTest.cpp + MdsGetManyTest.cpp # SKIP_TEST is not supported # MdsCallTest.cpp diff --git a/mdsobjects/cpp/testing/MdsGetManyTest.cpp b/mdsobjects/cpp/testing/MdsGetManyTest.cpp new file mode 100644 index 0000000000..6c5231bc26 --- /dev/null +++ b/mdsobjects/cpp/testing/MdsGetManyTest.cpp @@ -0,0 +1,137 @@ +/* +Copyright (c) 2026, Massachusetts Institute of Technology All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are met: + +Redistributions of source code must retain the above copyright notice, this +list of conditions and the following disclaimer. + +Redistributions in binary form must reproduce the above copyright notice, this +list of conditions and the following disclaimer in the documentation and/or +other materials provided with the distribution. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE +FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR +SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER +CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +*/ +#include + +#include +#include + +#include "testing.h" +#include "testutils/unique_ptr.h" + +using namespace MDSplus; +using namespace testing; + +// The mdsip server entry point for Connection.getMany() batches +// (tdi/remote/GetManyExecute.fun -> MdsObjectsCppShr->GetManyExecute) +extern "C" struct descriptor_xd *GetManyExecute(char *serializedIn); + +//////////////////////////////////////////////////////////////////////////////// +// GetMany results that evaluate to missing (issue #3070) //////////////////// +//////////////////////////////////////////////////////////////////////////////// + +int main() +{ + BEGIN_TESTING(GetMany); + + { // A Dictionary must accept a missing (NULL) value: getManyObj() stores + // one whenever a query succeeds with a nil result. The rest of the Apd + // machinery (constructor, propagateDeletion, convertToApdDsc) already + // NULL-guards its elements. + unique_ptr dict = new Dictionary(); + String *key = new String("value"); // freed by the Dictionary destructor + String lookup("value"); + + dict->setItem(key, NULL); + TEST1(dict->len() == 1); + TEST1(dict->getItem(&lookup) == NULL); + + // replace the missing value with data... + Int32 *num = new Int32(42); + dict->setItem(key, num); + unique_ptr value = dict->getItem(&lookup); + TEST1(value->getInt() == 42); + + // ...and the data with a missing value again + dict->setItem(key, NULL); + TEST1(dict->len() == 1); + TEST1(dict->getItem(&lookup) == NULL); + deleteData(num); + } + + { // A missing value must survive the serialize / deserialize round trip + // used to transport the GetManyExecute() result dictionary. + unique_ptr dict = new Dictionary(); + dict->setItem(new String("value"), NULL); + + int size = 0; + AutoArray serialized(dict->serialize(&size)); + TEST1(size > 0); + + unique_ptr back = (Dictionary *)deserialize(serialized.get()); + String lookup("value"); + TEST1(back->len() == 1); + TEST1(back->getItem(&lookup) == NULL); + } + + { // End to end: a GetMany query that SUCCEEDS with a nil result must come + // back as { 'value': missing }, matching what a plain get() of the same + // expression returns, instead of crashing the mdsip worker. + // The expression mirrors GETNCI(, "RECORD") of a clock whose + // record is a Range with a missing begin: * : * : 5E-6 + Dictionary *nilQuery = new Dictionary(); // freed by the List destructor + nilQuery->setItem(new String("name"), new String("nil")); + nilQuery->setItem(new String("exp"), + new String("DATA(BEGIN_OF(BUILD_RANGE(*, *, 5E-6)))")); + + Dictionary *numQuery = new Dictionary(); + numQuery->setItem(new String("name"), new String("num")); + numQuery->setItem(new String("exp"), new String("1 + 1")); + + unique_ptr queries = new List(); + queries->append(nilQuery); + queries->append(numQuery); + + int serSize = 0; + AutoArray serialized(queries->serialize(&serSize)); + + struct descriptor_xd *outXd = GetManyExecute(serialized.get()); + TEST1(outXd != NULL && outXd->pointer != NULL); + + // GetManyExecute() returns the result dictionary serialized into a + // byte array, just as it goes over the wire + struct descriptor_a *bytes = (struct descriptor_a *)outXd->pointer; + unique_ptr result = + (Dictionary *)deserialize((char *)bytes->pointer); + + String nilName("nil"), numName("num"), valueKey("value"), errorKey("error"); + + { // sanity: the numeric query evaluated normally + unique_ptr answer = (Dictionary *)result->getItem(&numName); + TEST1((Dictionary *)answer != NULL); + unique_ptr value = answer->getItem(&valueKey); + TEST1(value->getInt() == 2); + } + + { // the nil query: answered, not an error, and the value is missing + unique_ptr answer = (Dictionary *)result->getItem(&nilName); + TEST1((Dictionary *)answer != NULL); + TEST1(answer->getItem(&errorKey) == NULL); + TEST1(answer->len() == 1); // exactly one entry: 'value' + TEST1(answer->getItem(&valueKey) == NULL); // ...holding missing + } + } + + END_TESTING; +} From e066ce7dfbb5c784027ab7f804effc0cbeae0264 Mon Sep 17 00:00:00 2001 From: Darren Garnier Date: Tue, 21 Jul 2026 18:49:46 +1200 Subject: [PATCH 2/4] Fix: mdsip worker crash when a GetMany query succeeds with a nil result When a GetMany query evaluates successfully to nil, execute() returns a NULL Data* (convertFromDsc() maps an empty XD to NULL), and getManyObj() stores it under the 'value' key of the answer dictionary. That call, Dictionary::setItem, dereferenced the value unconditionally (data->incRefCount()), crashing the mdsip worker mid-reply. A plain get() of the same expression harmlessly returns nil, and queries that FAIL inside a GetMany are handled (an 'error' entry is returned) - only the successful-nil case was fatal. NULL-guard the value refcounting in Dictionary::setItem and getItem. Everything below already treats a NULL Apd element as missing: the Apd constructor and propagateDeletion() guard it, convertToApdDsc() emits a null descriptor pointer for it, MdsSerializeDscOut() writes it as offset 0, and both python clients deserialize it back as None/missing - so the answer arrives as { 'value': * }, matching plain get(). Fixes #3070 Co-Authored-By: Claude Fable 5 --- include/mdsobjects.h | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/include/mdsobjects.h b/include/mdsobjects.h index 1a30b0bb99..7954a628fb 100644 --- a/include/mdsobjects.h +++ b/include/mdsobjects.h @@ -2892,13 +2892,17 @@ namespace MDSplus dtype = DTYPE_DICTIONARY; } + // A NULL value represents missing ("*"): it is what execute() returns + // for an expression that evaluates to nil, and convertToApdDsc() emits + // it as a null descriptor pointer Data *getItem(String *strData) { for (std::size_t i = 0; i < descs.size(); i += 2) { if (strData->equals(descs[i])) { - descs[i + 1]->incRefCount(); + if (descs[i + 1]) + descs[i + 1]->incRefCount(); return descs[i + 1]; } } @@ -2915,9 +2919,11 @@ namespace MDSplus descs[i] = strData; strData->incRefCount(); - descs[i + 1]->decRefCount(); + if (descs[i + 1]) + descs[i + 1]->decRefCount(); descs[i + 1] = data; - data->incRefCount(); + if (data) + data->incRefCount(); return; } } @@ -2925,7 +2931,8 @@ namespace MDSplus descs.push_back(strData); descs.push_back(data); strData->incRefCount(); - data->incRefCount(); + if (data) + data->incRefCount(); } std::size_t len() { return Apd::len() / 2; } From 37252cbd99a11f692b127a0b3f65eba1886480bf Mon Sep 17 00:00:00 2001 From: Darren Garnier Date: Tue, 21 Jul 2026 21:29:09 +1200 Subject: [PATCH 3/4] changes to C testing harness for macOS not to hide segfaults --- testing/backends/check/lib/macos_timer.h | 10 +++++++++- testing/check_backend.c | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/testing/backends/check/lib/macos_timer.h b/testing/backends/check/lib/macos_timer.h index 68b0532526..e0fa15b28d 100644 --- a/testing/backends/check/lib/macos_timer.h +++ b/testing/backends/check/lib/macos_timer.h @@ -4,6 +4,8 @@ #include #include #include +#include +#include #include @@ -56,7 +58,13 @@ _timer_handler(void *arg) static inline void _default_timer_expiration(union sigval sv) { (void) sv; - signal(SIGALRM, NULL); + /* When timer_create() is called with a NULL sigevent (as the test + * harness does), a POSIX timer would deliver SIGALRM to the process on + * expiration. Emulate that here so the harness' SIGALRM handler runs and + * kills the timed-out child. The previous signal(SIGALRM, NULL) only reset + * the SIGALRM disposition and never raised it, so test timeouts silently + * never fired on macOS and a hung/crashed child stalled the runner. */ + kill(getpid(), SIGALRM); } static inline int diff --git a/testing/check_backend.c b/testing/check_backend.c index fd51d3a26b..d8085fa510 100644 --- a/testing/check_backend.c +++ b/testing/check_backend.c @@ -39,6 +39,10 @@ extern char *strsignal(int); #include #endif +#ifdef __APPLE__ +#include +#endif + #include #include #include @@ -758,6 +762,21 @@ int __setup_child() { setpgid(0, 0); group_pid = getpgrp(); + +#ifdef __APPLE__ + // On macOS a hardware fault (null deref, bad instruction, ...) is caught + // by the task-level Mach exception port (the system crash reporter), which + // parks the faulting thread instead of terminating the process. The child + // then never dies, the parent blocks forever in waitpid(), and the whole + // run stalls. Clearing the exception ports lets the fault fall through to + // the default action, so a crashing test dies promptly and is reported, + // just as on Linux. + task_set_exception_ports(mach_task_self(), + EXC_MASK_BAD_ACCESS | EXC_MASK_BAD_INSTRUCTION | + EXC_MASK_ARITHMETIC, + MACH_PORT_NULL, EXCEPTION_DEFAULT, 0); +#endif + return 1; } #endif From 8811fa21808d590cad8a2feab2573afbd97f0448 Mon Sep 17 00:00:00 2001 From: Darren Garnier Date: Tue, 21 Jul 2026 23:48:32 +1200 Subject: [PATCH 4/4] Fix: deleteData() must accept NULL so a nil GetMany result cannot crash deleteData(NULL) dereferenced the pointer at data->refCount. Make it a no-op on NULL, matching the "NULL == missing" convention the rest of the Apd/Dictionary machinery already follows. This closes the remaining crash on the args-less / nil-value GetMany paths (server getManyObj cleanup of an absent "args" key, and client GetMany::get() of a missing value). Co-Authored-By: Claude Opus 4.8 (1M context) --- mdsobjects/cpp/mdsdataobjects.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/mdsobjects/cpp/mdsdataobjects.cpp b/mdsobjects/cpp/mdsdataobjects.cpp index c17a17b127..8e35e869f6 100644 --- a/mdsobjects/cpp/mdsdataobjects.cpp +++ b/mdsobjects/cpp/mdsdataobjects.cpp @@ -330,6 +330,11 @@ Data::~Data() void MDSplus::deleteData(Data *data) { + // A NULL Data represents missing ("*"). deleteData() must accept it as a + // no-op: RAII wrappers (AutoData) and Apd::propagateDeletion() hand it the + // NULL that getItem()/execute() return for a nil result. + if (!data) + return; if (data->refCount <= 1) { if (data->units)