[Symphony] Suspicious Code
Konvicka Filip
Filip.Konvicka at logis.cz
Tue Sep 16 07:03:23 EDT 2014
Hi,
While cleaning some warnings I stumbled upon several very suspicious lines of code in Symphony 5.5.7.
Note that item 1 is a change in the headers, and unless this is fixed this causes warnings in programs that link libsymphony.
Items 2 - 4 are possible bugs in Symphony.
1. In OsiSolverInterface.hpp and OsiClpSolverInterface.hpp, there are functions defined with the default parameter values of std::string::npos. The type of this is size_t, but the argument for which the default is defined is unsigned. In 64bit builds, this leads to a truncation of the value.
virtual std::string getRowName(int rowIndex,
unsigned maxLen = static_cast<unsigned>(std::string::npos)) const;
I think that it is better to use
virtual std::string getRowName(int rowIndex,
unsigned maxLen = std::numeric_limits<unsigned>::max()) const;
If you look at how COIN_INT_MAX is defined in CoinFinite.hpp, this is seems to be an acceptable COIN coding practice. There are 5 occurrences in total of this (in OsiSolverInterface.hpp and OsiClpSolverInterface.hpp). The only use of this that I could find is OsiNames.cpp which uses this as a limit to std::string::substr. Also this change should not break the ABI since this does not affect the symbols.
2. In lp_heuristics.c, line 1410:
if(dive_depth > dive_depth_limit/2){
fix_incr_cnt =(int)(1.0*(frac_ip_cnt - d_fixed_cnt)/
(dive_depth_limit > (dive_depth ? dive_depth_limit - dive_depth : 1)));
if(fix_incr_cnt < 1) fix_incr_cnt = 1;
}
Look at the division. The divider is a bool value! (dive_depth_limit > (dive_depth ? dive_depth_limit - dive_depth : 1))
I think that most likely this should have been (dive_depth_limit > dive_depth) ? dive_depth_limit-dive_depth : 1, which by the way is equivalent to std::max(dive_depth_limit - dive_depth, 1).
if(dive_depth > dive_depth_limit/2){
fix_incr_cnt =(int)(1.0*(frac_ip_cnt - d_fixed_cnt)/
((dive_depth_limit > dive_depth) ? dive_depth_limit - dive_depth : 1));
if(fix_incr_cnt < 1) fix_incr_cnt = 1;
}
By the way, the warning that caught the compiler's attention was - lp_heuristics.c(1412): warning C4804: '/' : unsafe use of type 'bool' in operation
3. In master_prep_base.c:504, I can also see something very suspicious:
if(P->ub < (P->mip->obj_sense == SYM_MAXIMIZE) ? -(P->mip->obj_offset) :
P->mip->obj_offset){
This is actually evaluated as (P->ub < (P->mip->obj_sense == SYM_MAXIMIZE)) ? -(P->mip->obj_offset) : P->mip->obj_offset, but obviously the intention was
if(P->ub < ((P->mip->obj_sense == SYM_MAXIMIZE) ? -(P->mip->obj_offset) :
P->mip->obj_offset)){
Again the warning was master_prep_base.c(504): warning C4804: '<' : unsafe use of type 'bool' in operation
4. In master.c:231 and 233, there are assignments like this:
tm_par->warm_start_node_limit = (int)SYM_INFINITY;
warm_start_node_limit is an int, while SYM_INFINITY is defined as 1e20 (i.e. a double). This does not make much sense (the conversion results in some strange value), so I think that it's much safer to write
tm_par->warm_start_node_limit = INT_MAX;
Note that 1e20 mod (UINT_MAX+1) is the number that gets assigned, which is 1661992960 for 4byte ints but zero for 2byte ints. Lucky we don't have small ints anymore :-)
Cheers,
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.coin-or.org/pipermail/symphony/attachments/20140916/dba489ee/attachment.html>
More information about the Symphony
mailing list