#1199 smf: fault in parsing some combinations of parent and type in activationUnitTemplate
#1241 smf: node iterator is not stepped correctly
Smaller problems handled directly within this ticket.
A common patch is planned to be created. That will fix all the problems listed here.
List of problems:
Coverity tool shows memory leaks in SmfCampaignXmlParser::parseAdminOpAction when parsing "value" at "doAdminOperation" and "undoAdminOperation".
A simple solution could be to break out from loop when "value" is found. Also adding a comment that only single value is supported.
Last edit: Robert Apanowicz 2015-02-04
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
2. smfd_smfnd.c: smfnd_up(): there are 2 problems.
-return value not checked and caller function "proc_mds_info()" steps the "no_of_smfnd" counter anyway.
-memory leak in case when "saClmInitialize()" returns other than "SA_AIS_OK".
Proposed solution: free memory if "saClmInitialize()" returns other than "SA_AIS_OK". Add checking of return value of "smfnd_up()" in "proc_mds_info()", if not "SA_AIS_OK" then don't step the counter. Possibly add a warning log.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
3. smfa_utils.c: smfa_cbk_list_cleanup(): Coverity tool shows use-after-free problem. That could happen only when a special path is walked through, which in practice never happens.
Use-after-free at "prev_cbk->next_cbk" in the else case.
It could happen only if the previous round of the loop went to the "if" case and did "prev_cbk = cbk_list".
The proposed solution is to introduce an extra checking:
4. smfnd_mds.c: mds_rcv(): mixed-enums problem where "m_NCS_IPC_SEND()" method is used with wrong enum type for "prio". Instead of "NCS_IPC_PRIORITY", "MDS_SEND_PRIORITY_TYPE" was used.
For the values it does not make problem. Proposal is to cast the prio to "NCS_IPC_PRIORITY" when calling "m_NCS_IPC_SEND()".
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
5. some places: setenv(): return value not checked.
6. smfsv_evt.c: smf_enc_cbk_req(): branch_past_initialization problem for "length" variable: this problem is shown only by coverity so far. It is better to fix since it can cause problems with some compilers.
Proposal is to initialize at the begining of the function. That means it will be initialized and maybe not used (some of the goto cases). Other: variable "rc" not needed.
7. smfd_smfnd: smfnd_up(): this method has 2 pairs of mutex lock/unlock. The data is unreliable between these. Solution is to keep the first lock and last unlock. Also introduce unlocks at each return.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
8. SmfUpgradeStep::modifyInformationModel(): logically dead code at the end of while loop. If statement handles 3 cases (continue/return/break) where none of them let to reach the bottom of while loop. Coverity complaining about not running the destructor of the instance of SmfRollbackCcb (constructed each turn of the loop). Possible solution is to not construct each turn, since data not modified, but move the contructing before the loop.
9. SmfProcedureThread::handleEvents(): return value of "handleEvents" and "deleteImmHandle" not checked.
10. SmfUpgradeProcedure::createImmStep(): return value of "find" not checked. Without checking the result of "find", "pos" value is subtracted from it, then that value is provided directly to "substr" as input argument. For functionality it does not make a problem, but the substraction is unnecessary if "find" returns "npos".
11. SmfStepTypeNodeReboot::rollback(): "checkAndInvokeCallback()" called twice after each other which is an obvious fault. (Coverity: at the first call the return value is not checked).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
8. SmfUpgradeStep::modifyInformationModel(): there is no logically dead code at the end of while loop. Constructing of SmfRollbackCcb cannot be moved out from the while loop and put before it, because "doImmOperations" adds to it each turn of the loop.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Separate tickets created for 2 defects:
#1199 smf: fault in parsing some combinations of parent and type in activationUnitTemplate
#1241 smf: node iterator is not stepped correctly
Smaller problems handled directly within this ticket.
A common patch is planned to be created. That will fix all the problems listed here.
List of problems:
A simple solution could be to break out from loop when "value" is found. Also adding a comment that only single value is supported.
Last edit: Robert Apanowicz 2015-02-04
2. smfd_smfnd.c: smfnd_up(): there are 2 problems.
-return value not checked and caller function "proc_mds_info()" steps the "no_of_smfnd" counter anyway.
-memory leak in case when "saClmInitialize()" returns other than "SA_AIS_OK".
Proposed solution: free memory if "saClmInitialize()" returns other than "SA_AIS_OK". Add checking of return value of "smfnd_up()" in "proc_mds_info()", if not "SA_AIS_OK" then don't step the counter. Possibly add a warning log.
3. smfa_utils.c: smfa_cbk_list_cleanup(): Coverity tool shows use-after-free problem. That could happen only when a special path is walked through, which in practice never happens.
Use-after-free at "prev_cbk->next_cbk" in the else case.
It could happen only if the previous round of the loop went to the "if" case and did "prev_cbk = cbk_list".
The proposed solution is to introduce an extra checking:
prev_cbk = cbk_list; }else { free(cbk_list);- cbk_list = prev_cbk->next_cbk;
+ cbk_list = NULL;
+ if(prev_cbk)
+ cbk_list = prev_cbk->next_cbk;
}
}
4. smfnd_mds.c: mds_rcv(): mixed-enums problem where "m_NCS_IPC_SEND()" method is used with wrong enum type for "prio". Instead of "NCS_IPC_PRIORITY", "MDS_SEND_PRIORITY_TYPE" was used.
For the values it does not make problem. Proposal is to cast the prio to "NCS_IPC_PRIORITY" when calling "m_NCS_IPC_SEND()".
5. some places: setenv(): return value not checked.
6. smfsv_evt.c: smf_enc_cbk_req(): branch_past_initialization problem for "length" variable: this problem is shown only by coverity so far. It is better to fix since it can cause problems with some compilers.
Proposal is to initialize at the begining of the function. That means it will be initialized and maybe not used (some of the goto cases). Other: variable "rc" not needed.
7. smfd_smfnd: smfnd_up(): this method has 2 pairs of mutex lock/unlock. The data is unreliable between these. Solution is to keep the first lock and last unlock. Also introduce unlocks at each return.
8. SmfUpgradeStep::modifyInformationModel(): logically dead code at the end of while loop. If statement handles 3 cases (continue/return/break) where none of them let to reach the bottom of while loop. Coverity complaining about not running the destructor of the instance of SmfRollbackCcb (constructed each turn of the loop). Possible solution is to not construct each turn, since data not modified, but move the contructing before the loop.
9. SmfProcedureThread::handleEvents(): return value of "handleEvents" and "deleteImmHandle" not checked.
10. SmfUpgradeProcedure::createImmStep(): return value of "find" not checked. Without checking the result of "find", "pos" value is subtracted from it, then that value is provided directly to "substr" as input argument. For functionality it does not make a problem, but the substraction is unnecessary if "find" returns "npos".
11. SmfStepTypeNodeReboot::rollback(): "checkAndInvokeCallback()" called twice after each other which is an obvious fault. (Coverity: at the first call the return value is not checked).
8. SmfUpgradeStep::modifyInformationModel(): there is no logically dead code at the end of while loop. Constructing of SmfRollbackCcb cannot be moved out from the while loop and put before it, because "doImmOperations" adds to it each turn of the loop.
changeset: 6293:dcea068b38c2
tag: tip
user: Robert Apanowicz robert.apanowicz@ericsson.com
date: Thu Feb 19 07:43:53 2015 +0100
summary: smf: defects detected by Coverity tool fixed [#1154]
Related
Tickets:
#1154Last edit: Ingvar Bergström 2015-03-02
33 defects pointed out by Coverity Tool for SMF in default branch.
7 defects ignored since those were not relevant.
26 defects fixed by the scope of this ticket.
Coverity tool doesn't show any defects for SMF in default branch at the moment.