From dfe3d648da24196239b5af16ecbe60dd2455fbd3 Mon Sep 17 00:00:00 2001 From: David Wagner Date: Wed, 22 Jul 2015 15:56:12 +0200 Subject: [PATCH 1/2] RFC: modify the criterion APIs to take uints instead of ints Using signed integers in the criterion API made it necessary to add guards for undefined behaviour occuring when bit-shifting a signed integer up to its sign bit. It also artifically reduced by 1 the number of possible inclusive values in an inclusive criterion. All APIs are changed in a retrocompatible fashion to take unsigned ints as input parameters. However, getNumericalValue takes an output parameter. This one must be kept as it is. We add a overloaded version of getNumericalValue taking an unsigned int and depracate the "signed int" version. Users that do not update their code to pass unsigned integers will still work fine but will have the "31 inclusive criterion values" limitation. Signed-off-by: David Wagner --- bindings/c/ParameterFramework.cpp | 9 ++-- bindings/c/Test.cpp | 10 ++-- bindings/python/pfw.i | 8 +-- parameter/SelectionCriterionRule.cpp | 18 +++---- parameter/SelectionCriterionRule.h | 2 +- parameter/SelectionCriterionType.cpp | 50 +++++++++---------- parameter/SelectionCriterionType.h | 16 +++--- .../include/SelectionCriterionTypeInterface.h | 14 ++++-- test/test-platform/TestPlatform.cpp | 10 ++-- 9 files changed, 72 insertions(+), 65 deletions(-) diff --git a/bindings/c/ParameterFramework.cpp b/bindings/c/ParameterFramework.cpp index efc7d9922..164b19874 100644 --- a/bindings/c/ParameterFramework.cpp +++ b/bindings/c/ParameterFramework.cpp @@ -39,6 +39,7 @@ #include #include #include +#include using std::string; @@ -184,14 +185,14 @@ bool PfwHandler::createCriteria(const PfwCriterion criteriaArray[], size_t crite assert(type != NULL); // Add criterion values for (size_t valueIndex = 0; criterion.values[valueIndex] != NULL; ++valueIndex) { - int value; + unsigned int value; if (criterion.inclusive) { - // Check that (int)1 << valueIndex would not overflow (UB) - if(std::numeric_limits::max() >> valueIndex == 0) { + // Check that we are not going to shift too far (UB) + if (valueIndex >= sizeof(unsigned int) * CHAR_BIT) { return status.failure("Too many values for criterion " + string(criterion.name)); } - value = 1 << valueIndex; + value = 1U << valueIndex; } else { value = valueIndex; } diff --git a/bindings/c/Test.cpp b/bindings/c/Test.cpp index 8cbaa902e..7123f64b5 100644 --- a/bindings/c/Test.cpp +++ b/bindings/c/Test.cpp @@ -204,8 +204,8 @@ TEST_CASE_METHOD(Test, "Parameter-framework c api use") { } GIVEN("A criteria with lots of values") { - // Build a criterion with as many value as there is bits in int. - std::vector names(sizeof(int) * CHAR_BIT + 1, 'a'); + // Build a criterion with as many value as there is bits in unsigned int + 1 + std::vector names(sizeof(unsigned int) * CHAR_BIT + 2, 'a'); names.back() = '\0'; std::vector values(names.size()); for(size_t i = 0; i < values.size(); ++i) { @@ -238,14 +238,14 @@ TEST_CASE_METHOD(Test, "Parameter-framework c api use") { * values[n] = NULL * */ - const PfwCriterion duplicatedCriteria[] = {{"name", true, &values[0]}}; + const PfwCriterion longCriteria[] = {{"name", true, &values[0]}}; WHEN("The pfw is started with a too long criterion state list") { - REQUIRE_FAILURE(pfwStart(pfw, config, duplicatedCriteria, 1, &logger)); + REQUIRE_FAILURE(pfwStart(pfw, config, longCriteria, 1, &logger)); } WHEN("The pfw is started with max length criterion state list") { values[values.size() - 2] = NULL; // Hide last value - REQUIRE_SUCCESS(pfwStart(pfw, config, duplicatedCriteria, 1, &logger)); + REQUIRE_SUCCESS(pfwStart(pfw, config, longCriteria, 1, &logger)); } } diff --git a/bindings/python/pfw.i b/bindings/python/pfw.i index a3e58125c..34a9c2cb3 100644 --- a/bindings/python/pfw.i +++ b/bindings/python/pfw.i @@ -201,11 +201,11 @@ class ISelectionCriterionTypeInterface %} public: - virtual bool addValuePair(int iValue, const std::string& strValue) = 0; - virtual bool getNumericalValue(const std::string& strValue, int& iValue) const = 0; - virtual bool getLiteralValue(int iValue, std::string& strValue) const = 0; + virtual bool addValuePair(unsigned int uiValue, const std::string& strValue) = 0; + virtual bool getNumericalValue(const std::string& strValue, unsigned int& uiValue) const = 0; + virtual bool getLiteralValue(unsigned int uiValue, std::string& strValue) const = 0; virtual bool isTypeInclusive() const = 0; - virtual std::string getFormattedState(int iValue) const = 0; + virtual std::string getFormattedState(unsigned int uiValue) const = 0; protected: virtual ~ISelectionCriterionTypeInterface() {} diff --git a/parameter/SelectionCriterionRule.cpp b/parameter/SelectionCriterionRule.cpp index c376bb303..7187a6ddd 100644 --- a/parameter/SelectionCriterionRule.cpp +++ b/parameter/SelectionCriterionRule.cpp @@ -47,7 +47,7 @@ const CSelectionCriterionRule::SMatchingRuleDescription CSelectionCriterionRule: { "Excludes", false } }; -CSelectionCriterionRule::CSelectionCriterionRule() : _pSelectionCriterion(NULL), _eMatchesWhen(CSelectionCriterionRule::EIs), _iMatchValue(0) +CSelectionCriterionRule::CSelectionCriterionRule() : _pSelectionCriterion(NULL), _eMatchesWhen(CSelectionCriterionRule::EIs), _uiMatchValue(0) { } @@ -104,7 +104,7 @@ bool CSelectionCriterionRule::parse(CRuleParser& ruleParser, string& strError) } // Value - if (!_pSelectionCriterion->getCriterionType()->getNumericalValue(strValue, _iMatchValue)) { + if (!_pSelectionCriterion->getCriterionType()->getNumericalValue(strValue, _uiMatchValue)) { strError = "Value error: \"" + strValue + "\" is not part of criterion \"" + _pSelectionCriterion->getCriterionName() + "\""; @@ -126,7 +126,7 @@ void CSelectionCriterionRule::dump(string& strResult) const strResult += " "; // Value string strValue; - _pSelectionCriterion->getCriterionType()->getLiteralValue(_iMatchValue, strValue); + _pSelectionCriterion->getCriterionType()->getLiteralValue(_uiMatchValue, strValue); strResult += strValue; } @@ -137,13 +137,13 @@ bool CSelectionCriterionRule::matches() const switch(_eMatchesWhen) { case EIs: - return _pSelectionCriterion->is(_iMatchValue); + return _pSelectionCriterion->is(_uiMatchValue); case EIsNot: - return _pSelectionCriterion->isNot(_iMatchValue); + return _pSelectionCriterion->isNot(_uiMatchValue); case EIncludes: - return _pSelectionCriterion->includes(_iMatchValue); + return _pSelectionCriterion->includes(_uiMatchValue); case EExcludes: - return _pSelectionCriterion->excludes(_iMatchValue); + return _pSelectionCriterion->excludes(_uiMatchValue); default: assert(0); return false; @@ -183,7 +183,7 @@ bool CSelectionCriterionRule::fromXml(const CXmlElement& xmlElement, CXmlSeriali // Get Value string strValue = xmlElement.getAttributeString("Value"); - if (!_pSelectionCriterion->getCriterionType()->getNumericalValue(strValue, _iMatchValue)) { + if (!_pSelectionCriterion->getCriterionType()->getNumericalValue(strValue, _uiMatchValue)) { xmlDomainImportContext.setError("Wrong Value attribute value " + strValue + " in " + getKind() + " " + xmlElement.getPath()); @@ -210,7 +210,7 @@ void CSelectionCriterionRule::toXml(CXmlElement& xmlElement, CXmlSerializingCont // Set Value string strValue; - _pSelectionCriterion->getCriterionType()->getLiteralValue(_iMatchValue, strValue); + _pSelectionCriterion->getCriterionType()->getLiteralValue(_uiMatchValue, strValue); xmlElement.setAttributeString("Value", strValue); } diff --git a/parameter/SelectionCriterionRule.h b/parameter/SelectionCriterionRule.h index 70bddc2cb..cfc693bb1 100644 --- a/parameter/SelectionCriterionRule.h +++ b/parameter/SelectionCriterionRule.h @@ -87,7 +87,7 @@ class CSelectionCriterionRule : public CRule MatchesWhen _eMatchesWhen; // Value - int32_t _iMatchValue; + uint32_t _uiMatchValue; // Used for XML MatchesWhen attribute parsing static const SMatchingRuleDescription _astMatchesWhen[ENbMatchesWhen]; diff --git a/parameter/SelectionCriterionType.cpp b/parameter/SelectionCriterionType.cpp index f8463c4da..7b49d6acd 100644 --- a/parameter/SelectionCriterionType.cpp +++ b/parameter/SelectionCriterionType.cpp @@ -49,12 +49,12 @@ std::string CSelectionCriterionType::getKind() const } // From ISelectionCriterionTypeInterface -bool CSelectionCriterionType::addValuePair(int iValue, const std::string& strValue) +bool CSelectionCriterionType::addValuePair(unsigned int uiValue, const std::string& strValue) { // Check 1 bit set only for inclusive types - if (_bInclusive && (!iValue || (iValue & (iValue - 1)))) { + if (_bInclusive && (!uiValue || (uiValue & (uiValue - 1)))) { - log_warning("Rejecting value pair association: 0x%X - %s for Selection Criterion Type %s", iValue, strValue.c_str(), getName().c_str()); + log_warning("Rejecting value pair association: 0x%X - %s for Selection Criterion Type %s", uiValue, strValue.c_str(), getName().c_str()); return false; } @@ -62,68 +62,68 @@ bool CSelectionCriterionType::addValuePair(int iValue, const std::string& strVal // Check already inserted if (_numToLitMap.find(strValue) != _numToLitMap.end()) { - log_warning("Rejecting value pair association (literal already present): 0x%X - %s for Selection Criterion Type %s", iValue, strValue.c_str(), getName().c_str()); + log_warning("Rejecting value pair association (literal already present): 0x%X - %s for Selection Criterion Type %s", uiValue, strValue.c_str(), getName().c_str()); return false; } for (NumToLitMapConstIt it = _numToLitMap.begin(); it != _numToLitMap.end(); ++it) { - if (it->second == iValue) { + if (it->second == uiValue) { log_warning("Rejecting value pair association (numerical already present): 0x%X - %s" " for Selection Criterion Type %s", - iValue, strValue.c_str(), getName().c_str()); + uiValue, strValue.c_str(), getName().c_str()); return false; } } - _numToLitMap[strValue] = iValue; + _numToLitMap[strValue] = uiValue; return true; } -bool CSelectionCriterionType::getNumericalValue(const std::string& strValue, int& iValue) const +bool CSelectionCriterionType::getNumericalValue(const std::string& strValue, unsigned int& uiValue) const { if (_bInclusive) { Tokenizer tok(strValue, _strDelimiter); std::vector astrValues = tok.split(); size_t uiNbValues = astrValues.size(); - int iResult = 0; - size_t uiValueIndex; - iValue = 0; + unsigned int uiResult = 0; + size_t uuiValueIndex; + uiValue = 0; // Looping on each std::string delimited by "|" token and adding the associated value - for (uiValueIndex = 0; uiValueIndex < uiNbValues; uiValueIndex++) { + for (uuiValueIndex = 0; uuiValueIndex < uiNbValues; uuiValueIndex++) { - if (!getAtomicNumericalValue(astrValues[uiValueIndex], iResult)) { + if (!getAtomicNumericalValue(astrValues[uuiValueIndex], uiResult)) { return false; } - iValue |= iResult; + uiValue |= uiResult; } return true; } - return getAtomicNumericalValue(strValue, iValue); + return getAtomicNumericalValue(strValue, uiValue); } -bool CSelectionCriterionType::getAtomicNumericalValue(const std::string& strValue, int& iValue) const +bool CSelectionCriterionType::getAtomicNumericalValue(const std::string& strValue, unsigned int& uiValue) const { NumToLitMapConstIt it = _numToLitMap.find(strValue); if (it != _numToLitMap.end()) { - iValue = it->second; + uiValue = it->second; return true; } return false; } -bool CSelectionCriterionType::getLiteralValue(int iValue, std::string& strValue) const +bool CSelectionCriterionType::getLiteralValue(unsigned int uiValue, std::string& strValue) const { NumToLitMapConstIt it; for (it = _numToLitMap.begin(); it != _numToLitMap.end(); ++it) { - if (it->second == iValue) { + if (it->second == uiValue) { strValue = it->first; @@ -164,7 +164,7 @@ std::string CSelectionCriterionType::listPossibleValues() const } // Formatted state -std::string CSelectionCriterionType::getFormattedState(int iValue) const +std::string CSelectionCriterionType::getFormattedState(unsigned int uiValue) const { std::string strFormattedState; @@ -174,12 +174,12 @@ std::string CSelectionCriterionType::getFormattedState(int iValue) const uint32_t uiBit; bool bFirst = true; - for (uiBit = 0; uiBit < sizeof(iValue) * 8; uiBit++) { + for (uiBit = 0; uiBit < sizeof(uiValue) * 8; uiBit++) { - int iSingleBitValue = iValue & (1 << uiBit); + unsigned int uiSingleBitValue = uiValue & (1 << uiBit); // Check if current bit is set - if (!iSingleBitValue) { + if (!uiSingleBitValue) { continue; } @@ -187,7 +187,7 @@ std::string CSelectionCriterionType::getFormattedState(int iValue) const // Simple translation std::string strSingleValue; - if (!getLiteralValue(iSingleBitValue, strSingleValue)) { + if (!getLiteralValue(uiSingleBitValue, strSingleValue)) { // Numeric value not part supported values for this criterion type. continue; } @@ -204,7 +204,7 @@ std::string CSelectionCriterionType::getFormattedState(int iValue) const } else { // Simple translation - getLiteralValue(iValue, strFormattedState); + getLiteralValue(uiValue, strFormattedState); } // Sometimes nothing is set diff --git a/parameter/SelectionCriterionType.h b/parameter/SelectionCriterionType.h index ef4176a3f..7cb582e28 100644 --- a/parameter/SelectionCriterionType.h +++ b/parameter/SelectionCriterionType.h @@ -36,32 +36,32 @@ class CSelectionCriterionType : public CElement, public ISelectionCriterionTypeInterface { - typedef std::map::const_iterator NumToLitMapConstIt; + typedef std::map::const_iterator NumToLitMapConstIt; public: CSelectionCriterionType(bool bIsInclusive); // From ISelectionCriterionTypeInterface - virtual bool addValuePair(int iValue, const std::string& strValue); + virtual bool addValuePair(unsigned int uiValue, const std::string& strValue); /** * Retrieve the numerical value from the std::string representation of the criterion type. * * @param[in] strValue: criterion type value represented as a stream. If the criterion is * inclusive, it supports more than one criterion type value delimited * by the "|" symbol. - * @param[out] iValue: criterion type value represented as an integer. + * @param[out] uiValue: criterion type value represented as an unsigned integer. * * @return true if integer value retrieved from the std::string one, false otherwise. */ - virtual bool getNumericalValue(const std::string& strValue, int& iValue) const; - virtual bool getLiteralValue(int iValue, std::string& strValue) const; + virtual bool getNumericalValue(const std::string& strValue, unsigned int& uiValue) const; + virtual bool getLiteralValue(unsigned int uiValue, std::string& strValue) const; virtual bool isTypeInclusive() const; // Value list std::string listPossibleValues() const; // Formatted state - virtual std::string getFormattedState(int iValue) const; + virtual std::string getFormattedState(unsigned int uiValue) const; /** * Export to XML @@ -84,9 +84,9 @@ class CSelectionCriterionType : public CElement, public ISelectionCriterionTypeI * * @return true if integer value retrieved from the std::string one, false otherwise. */ - bool getAtomicNumericalValue(const std::string& strValue, int& iValue) const; + bool getAtomicNumericalValue(const std::string& strValue, unsigned int& uiValue) const; bool _bInclusive; - std::map _numToLitMap; + std::map _numToLitMap; static const std::string _strDelimiter; /**< Inclusive criterion type delimiter. */ }; diff --git a/parameter/include/SelectionCriterionTypeInterface.h b/parameter/include/SelectionCriterionTypeInterface.h index 0b62ee46a..ba6e60b7e 100644 --- a/parameter/include/SelectionCriterionTypeInterface.h +++ b/parameter/include/SelectionCriterionTypeInterface.h @@ -34,11 +34,17 @@ class ISelectionCriterionTypeInterface { public: - virtual bool addValuePair(int iValue, const std::string& strValue) = 0; - virtual bool getNumericalValue(const std::string& strValue, int& iValue) const = 0; - virtual bool getLiteralValue(int iValue, std::string& strValue) const = 0; + virtual bool addValuePair(unsigned int uiValue, const std::string& strValue) = 0; + virtual bool getNumericalValue(const std::string& strValue, unsigned int& uiValue) const = 0; + virtual bool getLiteralValue(unsigned int iValue, std::string& strValue) const = 0; virtual bool isTypeInclusive() const = 0; - virtual std::string getFormattedState(int iValue) const = 0; + virtual std::string getFormattedState(unsigned int iValue) const = 0; + + /** deprecated, use getNumericalValue(const std::string& unsigned int&) instead */ + bool getNumericalValue(const std::string& strValue, int& iValue) const + { + return getNumericalValue(strValue, (unsigned int&)iValue); + }; protected: virtual ~ISelectionCriterionTypeInterface() {} diff --git a/test/test-platform/TestPlatform.cpp b/test/test-platform/TestPlatform.cpp index 9ccac1ba9..88463f3e3 100644 --- a/test/test-platform/TestPlatform.cpp +++ b/test/test-platform/TestPlatform.cpp @@ -339,7 +339,7 @@ bool CTestPlatform::createInclusiveSelectionCriterionFromStateList( const std::string& strValue = remoteCommand.getArgument(uiState + 1); - if (!pCriterionType->addValuePair(0x1 << uiState, strValue)) { + if (!pCriterionType->addValuePair(1U << uiState, strValue)) { strResult = "Unable to add value: " + strValue; @@ -398,7 +398,7 @@ bool CTestPlatform::createInclusiveSelectionCriterion(const string& strName, ostrValue << "State_0x"; ostrValue << (0x1 << uiState); - if (!pCriterionType->addValuePair(0x1 << uiState, ostrValue.str())) { + if (!pCriterionType->addValuePair(1U << uiState, ostrValue.str())) { strResult = "Unable to add value: " + ostrValue.str(); @@ -460,7 +460,7 @@ bool CTestPlatform::setCriterionStateByLexicalSpace(const IRemoteCommand& remote } /// Translate lexical state to numerical state - int iNumericalState = 0; + unsigned int uiNumericalState = 0; uint32_t uiLexicalSubStateIndex; // Parse lexical substates @@ -482,7 +482,7 @@ bool CTestPlatform::setCriterionStateByLexicalSpace(const IRemoteCommand& remote } // Translate lexical to numerical substate - if (!pCriterionType->getNumericalValue(strLexicalState, iNumericalState)) { + if (!pCriterionType->getNumericalValue(strLexicalState, uiNumericalState)) { strResult = "Unable to find lexical state \"" + strLexicalState + "\" in criteria " + strCriterionName; @@ -491,7 +491,7 @@ bool CTestPlatform::setCriterionStateByLexicalSpace(const IRemoteCommand& remote } // Set criterion new state - pCriterion->setCriterionState(iNumericalState); + pCriterion->setCriterionState(uiNumericalState); return true; } From c7c104d58ba25f0a16a7be6acf5e2d60d9b77bde Mon Sep 17 00:00:00 2001 From: David Wagner Date: Wed, 22 Jul 2015 16:10:21 +0200 Subject: [PATCH 2/2] test-platform: limit the number of inclusive criterion values to 32 Otherwise, we'd trigger an undefined behaviour by shifting a unsigned int more than its size. Signed-off-by: David Wagner --- test/test-platform/TestPlatform.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/test-platform/TestPlatform.cpp b/test/test-platform/TestPlatform.cpp index 88463f3e3..b9029d50c 100644 --- a/test/test-platform/TestPlatform.cpp +++ b/test/test-platform/TestPlatform.cpp @@ -333,6 +333,14 @@ bool CTestPlatform::createInclusiveSelectionCriterionFromStateList( assert(pCriterionType != NULL); uint32_t uiNbStates = remoteCommand.getArgumentCount() - 1; + + if (uiNbStates > 32) { + + strResult = "Maximum number of states for inclusive criterion is 32"; + + return false; + } + uint32_t uiState; for (uiState = 0; uiState < uiNbStates; uiState++) { @@ -389,6 +397,13 @@ bool CTestPlatform::createInclusiveSelectionCriterion(const string& strName, ISelectionCriterionTypeInterface* pCriterionType = _pParameterMgrPlatformConnector->createSelectionCriterionType(true); + if (uiNbStates > 32) { + + strResult = "Maximum number of states for inclusive criterion is 32"; + + return false; + } + uint32_t uiState; for (uiState = 0; uiState < uiNbStates; uiState++) {