From 98bf517cfccf3016177a66e3b8cb6f70da1f94a6 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Mon, 7 Mar 2022 15:47:05 -0600 Subject: [PATCH 1/5] Add unit tests for MMCore crashes Most of these fail (except APIErrorTests.GetPixelSizeUmWithNoConfig). --- MMCore/unittest/APIError-Tests.cpp | 93 ++++++++++++++++++++++++++++++ MMCore/unittest/Makefile.am | 1 + 2 files changed, 94 insertions(+) create mode 100644 MMCore/unittest/APIError-Tests.cpp diff --git a/MMCore/unittest/APIError-Tests.cpp b/MMCore/unittest/APIError-Tests.cpp new file mode 100644 index 000000000..94b0557e1 --- /dev/null +++ b/MMCore/unittest/APIError-Tests.cpp @@ -0,0 +1,93 @@ +#include + +#include "MMCore.h" + +TEST(APIErrorTests, SetFocusDirectionWithInvalidDevice) +{ + CMMCore c; + // Must not throw or crash + c.setFocusDirection(nullptr, 0); + c.setFocusDirection("", 0); + c.setFocusDirection("Blah", 0); + c.setFocusDirection("Core", 0); + c.setFocusDirection(nullptr, +1); + c.setFocusDirection("", +1); + c.setFocusDirection("Blah", +1); + c.setFocusDirection("Core", +1); + c.setFocusDirection(nullptr, -1); + c.setFocusDirection("", -1); + c.setFocusDirection("Blah", -1); + c.setFocusDirection("Core", -1); +} + +TEST(APIErrorTests, GetNumberOfStatesWithInvalidDevice) +{ + CMMCore c; + // Must not throw or crash + EXPECT_EQ(-1, c.getNumberOfStates(nullptr)); + EXPECT_EQ(-1, c.getNumberOfStates("")); + EXPECT_EQ(-1, c.getNumberOfStates("Blah")); + EXPECT_EQ(-1, c.getNumberOfStates("Core")); +} + +TEST(APIErrorTests, GetAvailableConfigsWithInvalidGroup) +{ + CMMCore c; + EXPECT_TRUE(c.getAvailableConfigs(nullptr).empty()); + EXPECT_TRUE(c.getAvailableConfigs("").empty()); + EXPECT_TRUE(c.getAvailableConfigs("Blah").empty()); +} + +TEST(APIErrorTests, GetPixelSizeUmWithNoConfig) +{ + CMMCore c; + EXPECT_EQ(0.0, c.getPixelSizeUm(false)); + EXPECT_EQ(0.0, c.getPixelSizeUm(true)); +} + +TEST(APIErrorTests, IsConfigDefinedWithNullArgs) +{ + CMMCore c; + EXPECT_FALSE(c.isConfigDefined(nullptr, "Blah")); + EXPECT_FALSE(c.isConfigDefined("Blah", nullptr)); + EXPECT_FALSE(c.isConfigDefined(nullptr, nullptr)); + EXPECT_FALSE(c.isConfigDefined("Blah", "Blah")); + EXPECT_FALSE(c.isConfigDefined(nullptr, "")); + EXPECT_FALSE(c.isConfigDefined("", nullptr)); + EXPECT_FALSE(c.isConfigDefined(nullptr, nullptr)); + EXPECT_FALSE(c.isConfigDefined("", "")); + EXPECT_FALSE(c.isConfigDefined("", "Blah")); + EXPECT_FALSE(c.isConfigDefined("Blah", "")); +} + +TEST(APIErrorTests, IsGroupDefinedWithNullArg) +{ + CMMCore c; + EXPECT_FALSE(c.isGroupDefined(nullptr)); + EXPECT_FALSE(c.isGroupDefined("")); + EXPECT_FALSE(c.isGroupDefined("Blah")); +} + +TEST(APIErrorTests, SupportsDeviceDetectionWithInvalidDevice) +{ + CMMCore c; + EXPECT_FALSE(c.supportsDeviceDetection(nullptr)); + EXPECT_FALSE(c.supportsDeviceDetection("")); + EXPECT_FALSE(c.supportsDeviceDetection("Blah")); + EXPECT_FALSE(c.supportsDeviceDetection("Core")); +} + +TEST(APIErrorTests, DetectDeviceWithInvalidDevice) +{ + CMMCore c; + EXPECT_EQ(MM::Unimplemented, c.detectDevice(nullptr)); + EXPECT_EQ(MM::Unimplemented, c.detectDevice("")); + EXPECT_EQ(MM::Unimplemented, c.detectDevice("Blah")); + EXPECT_EQ(MM::Unimplemented, c.detectDevice("Core")); +} + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} \ No newline at end of file diff --git a/MMCore/unittest/Makefile.am b/MMCore/unittest/Makefile.am index 063b5d22d..0f6e0bbce 100644 --- a/MMCore/unittest/Makefile.am +++ b/MMCore/unittest/Makefile.am @@ -1,4 +1,5 @@ check_PROGRAMS = \ + APIError-Tests \ CoreSanity-Tests \ LoggingSplitEntryIntoLines-Tests \ Logger-Tests From 9afd47e07f4d394dbd41c0e01f3bf58ad07c070b Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Fri, 4 Mar 2022 17:28:08 -0600 Subject: [PATCH 2/5] Prevent MMCore crash by uncaught exceptions And, in one case, a null pointer. Did not fix PropertyBlock functions which should be deprecated. --- MMCore/MMCore.cpp | 110 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 91 insertions(+), 19 deletions(-) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index f4f68f23e..fef27c965 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -423,6 +423,11 @@ string CMMCore::getAPIVersionInfo() const /** * Returns the entire system state, i.e. the collection of all property values from all devices. + * + * For legacy reasons, this function does not throw an exception if there is an + * error. If there is an error, properties may be missing from the return + * value. + * * @return Configuration object containing a collection of device-property-value triplets */ Configuration CMMCore::getSystemState() @@ -1955,6 +1960,9 @@ int CMMCore::getFocusDirection(const char* stageLabel) throw (CMMError) * * Once this method is called, getFocusDirection() for the stage will always * return the set value. + * + * For legacy reasons, an exception is not thrown if there is an error. + * Instead, nothing is done if stageLabel is not a valid focus stage. */ void CMMCore::setFocusDirection(const char* stageLabel, int sign) { @@ -1964,11 +1972,17 @@ void CMMCore::setFocusDirection(const char* stageLabel, int sign) if (sign < 0) direction = MM::FocusDirectionAwayFromSample; - boost::shared_ptr stage = - deviceManager_->GetDeviceOfType(stageLabel); + try + { + boost::shared_ptr stage = + deviceManager_->GetDeviceOfType(stageLabel); - mm::DeviceModuleLockGuard guard(stage); - stage->SetFocusDirection(direction); + mm::DeviceModuleLockGuard guard(stage); + stage->SetFocusDirection(direction); + } + catch (const CMMError&) + { + } } @@ -4625,13 +4639,23 @@ long CMMCore::getState(const char* deviceLabel) throw (CMMError) /** * Returns the total number of available positions (states). + * + * For legacy reasons, an exception is not thrown on error. + * Instead, -1 is returned if deviceLabel is not a valid state device. */ long CMMCore::getNumberOfStates(const char* deviceLabel) { - boost::shared_ptr pStateDev = - deviceManager_->GetDeviceOfType(deviceLabel); - mm::DeviceModuleLockGuard guard(pStateDev); - return pStateDev->GetNumberOfPositions(); + try + { + boost::shared_ptr pStateDev = + deviceManager_->GetDeviceOfType(deviceLabel); + mm::DeviceModuleLockGuard guard(pStateDev); + return pStateDev->GetNumberOfPositions(); + } + catch (const CMMError&) + { + return -1; + } } /** @@ -5144,13 +5168,25 @@ void CMMCore::deleteConfig(const char* groupName, const char* configName, const /** * Returns all defined configuration names in a given group + * + * For legacy reasons, an exception is not thrown if there is an error. + * Instead, an empty vector is returned if group is not a valid config group. + * * @return an array of configuration names */ vector CMMCore::getAvailableConfigs(const char* group) const { - CheckConfigGroupName(group); + std::vector ret; + try + { + CheckConfigGroupName(group); - return configGroups_->GetAvailableConfigs(group); + ret = configGroups_->GetAvailableConfigs(group); + } + catch (const CMMError&) + { + } + return ret; } /** @@ -5399,14 +5435,29 @@ double CMMCore::getPixelSizeUm() * Returns the current pixel size in microns. * This method is based on sensing the current pixel size configuration and adjusting * for the binning. + * + * For legacy reasons, an exception is not thrown if there is an error. + * Instead, 0.0 is returned if any property values cannot be read, or if no + * pixel size preset matches the property values. */ double CMMCore::getPixelSizeUm(bool cached) { - std::string resolutionID = getCurrentPixelSizeConfig(cached); + std::string resolutionID; + try + { + resolutionID = getCurrentPixelSizeConfig(cached); + } + catch (const CMMError&) + { + } + if (resolutionID.length() > 0) { // check which one matches the current state PixelSizeConfiguration* pCfg = pixelSizeGroup_->Find(resolutionID.c_str()); + if (!pCfg) + return 0.0; + double pixSize = pCfg->getPixelSizeUm(); boost::shared_ptr camera = currentCameraDevice_.lock(); @@ -5561,8 +5612,8 @@ double CMMCore::getMagnificationFactor() const */ bool CMMCore::isConfigDefined(const char* groupName, const char* configName) { - CheckConfigGroupName(groupName); - CheckConfigPresetName(configName); + if (!groupName || !configName) + return false; return configGroups_->Find(groupName, configName) != 0; } @@ -5574,7 +5625,8 @@ bool CMMCore::isConfigDefined(const char* groupName, const char* configName) */ bool CMMCore::isGroupDefined(const char* groupName) { - CheckConfigGroupName(groupName); + if (!groupName) + return false; return configGroups_->isDefined(groupName); } @@ -7532,13 +7584,23 @@ void CMMCore::updateAllowedChannelGroups() /** * Return whether or not the device supports automatic device detection * (i.e. whether or not detectDevice() may be safely called). + * + * For legacy reasons, an exception is not thrown if there is an error. + * Instead, false is returned if label is not a valid device. */ bool CMMCore::supportsDeviceDetection(char* label) { - boost::shared_ptr pDevice = - deviceManager_->GetDevice(label); - mm::DeviceModuleLockGuard guard(pDevice); - return pDevice->SupportsDeviceDetection(); + try + { + boost::shared_ptr pDevice = + deviceManager_->GetDevice(label); + mm::DeviceModuleLockGuard guard(pDevice); + return pDevice->SupportsDeviceDetection(); + } + catch (const CMMError&) + { + return false; + } } /** @@ -7546,11 +7608,21 @@ bool CMMCore::supportsDeviceDetection(char* label) * Used to automate discovery of correct serial port * Also configures the serial port correctly * + * For legacy reasons, an exception is not thrown if there is an error. + * Instead, MM::Unimplemented is returned if label is not a valid device. + * * @param label the label of the device for which the serial port should be found */ MM::DeviceDetectionStatus CMMCore::detectDevice(char* label) { - CheckDeviceLabel(label); + try + { + CheckDeviceLabel(label); + } + catch (const CMMError&) + { + return MM::Unimplemented; + } MM::DeviceDetectionStatus result = MM::Unimplemented; std::vector< std::string> propertiesToRestore; From 9cca84e4efd656aa8eb8713733a47e260da5cd04 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Fri, 4 Mar 2022 17:37:13 -0600 Subject: [PATCH 3/5] Deprecate MMCore property block functions It is not clear to me what these were intended for, but they are probably part of a planned but unfinished feature (seeing that it appears to be tied to an Equipment command in config files, which, as far as I know, was never supported by the Hardware Configuration Wizard). Some of the functions deprecated here may crash when called with incorrect parameters. --- MMCore/MMCore.cpp | 10 ++++++++++ MMCore/MMCore.h | 14 +++++++------- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index fef27c965..e308bc2cb 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -5635,6 +5635,8 @@ bool CMMCore::isGroupDefined(const char* groupName) * Defines a reference for the collection of property-value pairs. * This construct is useful for defining * interchangeable equipment features, such as objective magnifications, filter wavelengths, etc. + * + * @deprecated Property blocks will not be supported in the future. */ void CMMCore::definePropertyBlock(const char* blockName, const char* propertyName, const char* propertyValue) { @@ -5663,6 +5665,8 @@ void CMMCore::definePropertyBlock(const char* blockName, const char* propertyNam /** * Returns all defined property block identifiers. + * + * @deprecated Property blocks will not be supported in the future. */ std::vector CMMCore::getAvailablePropertyBlocks() const { @@ -5676,6 +5680,8 @@ std::vector CMMCore::getAvailablePropertyBlocks() const /** * Returns the collection of property-value pairs defined in this block. + * + * @deprecated Property blocks will not be supported in the future. */ PropertyBlock CMMCore::getPropertyBlockData(const char* blockName) { @@ -5693,6 +5699,8 @@ PropertyBlock CMMCore::getPropertyBlockData(const char* blockName) /** * Returns the collection of property-value pairs defined for the specific device and state label. + * + * @deprecated Property blocks will not be supported in the future. */ PropertyBlock CMMCore::getStateLabelData(const char* deviceLabel, const char* stateLabel) { @@ -5725,6 +5733,8 @@ PropertyBlock CMMCore::getStateLabelData(const char* deviceLabel, const char* st /** * Returns the collection of property-value pairs defined for the current state. + * + * @deprecated Property blocks will not be supported in the future. */ PropertyBlock CMMCore::getData(const char* deviceLabel) { diff --git a/MMCore/MMCore.h b/MMCore/MMCore.h index d048344ee..bb6644995 100644 --- a/MMCore/MMCore.h +++ b/MMCore/MMCore.h @@ -345,10 +345,10 @@ class CMMCore /** \name Property blocks. */ ///@{ - void definePropertyBlock(const char* blockName, const char* propertyName, - const char* propertyValue); - std::vector getAvailablePropertyBlocks() const; - PropertyBlock getPropertyBlockData(const char* blockName); + MMCORE_DEPRECATED(void definePropertyBlock(const char* blockName, const char* propertyName, + const char* propertyValue)); + MMCORE_DEPRECATED(std::vector getAvailablePropertyBlocks() const); + MMCORE_DEPRECATED(PropertyBlock getPropertyBlockData(const char* blockName)); ///@} /** \name Image acquisition. */ @@ -464,9 +464,9 @@ class CMMCore throw (CMMError); long getStateFromLabel(const char* stateDeviceLabel, const char* stateLabel) throw (CMMError); - PropertyBlock getStateLabelData(const char* stateDeviceLabel, - const char* stateLabel); - PropertyBlock getData(const char* stateDeviceLabel); + MMCORE_DEPRECATED(PropertyBlock getStateLabelData(const char* stateDeviceLabel, + const char* stateLabel)); + MMCORE_DEPRECATED(PropertyBlock getData(const char* stateDeviceLabel)); ///@} /** \name Focus (Z) stage control. */ From fe13c34f272864e41d6f97aab7ea52cb7dbf8728 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Fri, 4 Mar 2022 17:44:48 -0600 Subject: [PATCH 4/5] Deprecate MMCore ImageSynchro functions Among other things, it doesn't even do what it says because it only applies to snap, not to sequence acquisition. ImageSynchro has not been used by MMStudio for a long time. Apps can simply wait for individual devices as needed. --- MMCore/MMCore.cpp | 8 ++++++++ MMCore/MMCore.h | 8 ++++---- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index e308bc2cb..76a8dbb4d 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -1316,6 +1316,8 @@ void CMMCore::waitForConfig(const char* group, const char* configName) throw (CM /** * Wait for the slowest device in the ImageSynchro list. + * + * @deprecated ImageSynchro will not be supported in the future. */ void CMMCore::waitForImageSynchro() throw (CMMError) { @@ -2469,6 +2471,8 @@ namespace * Add device to the image-synchro list. Image acquisition waits for all devices * in this list. * @param label the device label + * + * @deprecated ImageSynchro will not be supported in the future. */ void CMMCore::assignImageSynchro(const char* label) throw (CMMError) { @@ -2485,6 +2489,8 @@ void CMMCore::assignImageSynchro(const char* label) throw (CMMError) /** * Removes device from the image-synchro list. * @param label the device label + * + * @deprecated ImageSynchro will not be supported in the future. */ void CMMCore::removeImageSynchro(const char* label) throw (CMMError) { @@ -2497,6 +2503,8 @@ void CMMCore::removeImageSynchro(const char* label) throw (CMMError) /** * Clears the image synchro device list. + * + * @deprecated ImageSynchro will not be supported in the future. */ void CMMCore::removeImageSynchroAll() { diff --git a/MMCore/MMCore.h b/MMCore/MMCore.h index bb6644995..3c7fae6fb 100644 --- a/MMCore/MMCore.h +++ b/MMCore/MMCore.h @@ -242,7 +242,7 @@ class CMMCore void waitForConfig(const char* group, const char* configName) throw (CMMError); bool systemBusy() throw (CMMError); void waitForSystem() throw (CMMError); - void waitForImageSynchro() throw (CMMError); + MMCORE_DEPRECATED(void waitForImageSynchro() throw (CMMError)); bool deviceTypeBusy(MM::DeviceType devType) throw (CMMError); void waitForDeviceType(MM::DeviceType devType) throw (CMMError); @@ -386,9 +386,9 @@ class CMMCore std::string getCameraChannelName(unsigned int channelNr); long getImageBufferSize(); - void assignImageSynchro(const char* deviceLabel) throw (CMMError); - void removeImageSynchro(const char* deviceLabel) throw (CMMError); - void removeImageSynchroAll(); + MMCORE_DEPRECATED(void assignImageSynchro(const char* deviceLabel) throw (CMMError)); + MMCORE_DEPRECATED(void removeImageSynchro(const char* deviceLabel) throw (CMMError)); + MMCORE_DEPRECATED(void removeImageSynchroAll()); void setAutoShutter(bool state); bool getAutoShutter(); From 4f32845a338e6cf3a7435627cc934d9a0098fa78 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Mon, 7 Mar 2022 15:52:11 -0600 Subject: [PATCH 5/5] Increment MMCore minor version This is for the new guaranteed behavior on errors for a few functions, and the deprecation of PropertyBlock and ImageSynchro functions. --- MMCore/MMCore.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MMCore/MMCore.cpp b/MMCore/MMCore.cpp index 76a8dbb4d..7c6c11085 100644 --- a/MMCore/MMCore.cpp +++ b/MMCore/MMCore.cpp @@ -113,7 +113,7 @@ using namespace std; * (Keep the 3 numbers on one line to make it easier to look at diffs when * merging/rebasing.) */ -const int MMCore_versionMajor = 10, MMCore_versionMinor = 1, MMCore_versionPatch = 1; +const int MMCore_versionMajor = 10, MMCore_versionMinor = 2, MMCore_versionPatch = 0; ///////////////////////////////////////////////////////////////////////////////