Skip to content
This repository was archived by the owner on Aug 5, 2022. It is now read-only.

Add check for inclusive criterion overflow #146

Merged
dawagner merged 1 commit into from
Jul 13, 2015
Merged

Add check for inclusive criterion overflow #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion parameter/SelectionCriterionType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011-2014, Intel Corporation
* Copyright (c) 2011-2015, Intel Corporation
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without modification,
Expand Down Expand Up @@ -30,6 +30,8 @@
#include "SelectionCriterionType.h"
#include "Tokenizer.h"

#include <climits>

#define base CElement

const std::string CSelectionCriterionType::_strDelimiter = "|";
Expand All @@ -51,6 +53,18 @@ std::string CSelectionCriterionType::getKind() const
// From ISelectionCriterionTypeInterface
bool CSelectionCriterionType::addValuePair(int iValue, const std::string& strValue)
{
// An inclusive criterion is implemented as a bitfield over an int and
// thus, can't have values larger than the number of bits in an int.
Copy link
Contributor

@krocard krocard Jul 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not correct. How about:

 /* 
 Inclusive criterion is implemented as a bitfield over an int.
 As such fitfield are usually constructed by shifting a bit, forbid the sign bit to be used.
 Shifting a bit over the sign bit returns in an overflow, which is an undefined behavior.

 This check is more educative than effective, as to be detected the overflow might have already happed.
 The client might have generated the value safely (by casting from unsigned), but this is considered unlikely.
 */

Copy link
Contributor Author

@dawagner dawagner Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obsolete

static const unsigned int inclusiveCriterionMaxValue = 1 << (sizeof(iValue) * CHAR_BIT - 1);

if (_bInclusive && (unsigned int)iValue > inclusiveCriterionMaxValue) {
Copy link
Contributor

@krocard krocard Jul 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be equivalent to check < 0 ?

Copy link
Contributor Author

@dawagner dawagner Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obsolete


log_warning("Rejecting value pair association: 0x%X - %s "
"because an inclusive criterion can't have values larger than 0x%X",
iValue, strValue.c_str(), inclusiveCriterionMaxValue);
return false;
}

// Check 1 bit set only for inclusive types
if (_bInclusive && (!iValue || (iValue & (iValue - 1)))) {

Expand Down
17 changes: 1 addition & 16 deletions test/test-platform/TestPlatform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011-2014, Intel Corporation
* Copyright (c) 2011-2015, Intel Corporation
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without modification,
Expand Down Expand Up @@ -333,14 +333,6 @@ 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++) {
Expand Down Expand Up @@ -397,13 +389,6 @@ 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++) {
Expand Down