[Symphony] Suspicious Code

[Ted Ralphs]

Hi Filip,

Sorry for the delay. I made the fixes you pointed out and pushed them to
trunk and stable. The fixes for CoinUtils have already been released along
with some other fixes I made. I will have a new release of SYMPHONY
incorporating everything in a day or two. Thanks for the feedback!

Cheers,

Ted

On Thu, Sep 25, 2014 at 12:52 PM, Ted Ralphs <ted at lehigh.edu> wrote:

> Thanks for the report! I haven't had a chance to fix these issues yet, but
> I will do so soon.
>
> Cheers,
>
> Ted
>
> On Tue, Sep 16, 2014 at 7:03 AM, Konvicka Filip <Filip.Konvicka at logis.cz>
> wrote:
>
>>  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
>>
>>
>>
>> _______________________________________________
>> Symphony mailing list
>> Symphony at list.coin-or.org
>> http://list.coin-or.org/mailman/listinfo/symphony
>>
>>
>
>
> --
> Dr. Ted Ralphs
> Associate Professor, Lehigh University
> (610) 628-1280
> ted 'at' lehigh 'dot' edu
> coral.ie.lehigh.edu/~ted
>



-- 
Dr. Ted Ralphs
Professor, Lehigh University
(610) 628-1280
ted 'at' lehigh 'dot' edu
coral.ie.lehigh.edu/~ted
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.coin-or.org/pipermail/symphony/attachments/20141027/b4d54d93/attachment.html>