[Symphony] Suspicious Code
Ted Ralphs
ted at lehigh.edu
Wed Nov 5 18:36:57 EST 2014
Yes, I just noticed that if the pre-processor level is set to -1, then
SYMPHONY crashes. Level 0 doesn't do anything that would normally be
considered pre-processing, just some bookkeeping, and is probably the
lowest level that should probably be allowed. At level 0, I haven't seen
any problems. I will fix the issue in the next release. Feel free to follow
up with more information on the problem.
Cheers,
Ted
On Wed, Nov 5, 2014 at 5:50 PM, Konvicka Filip <Filip.Konvicka at logis.cz>
wrote:
> Ted,
>
>
>
> Thanks very much! And no need to apologize, you’re still very fast. If
> there’s anything else I’ll try to post another message. One thing I
> noticed was that if preprocessing is turned off weird stuff happens...I was
> trying to keep my row/column numbering but the preprocess step prevented
> that – so I tried to turn it off, but I think the program crashed. I’ll
> try to reproduce this.
>
>
>
> Thanks again,
>
> Filip
>
>
>
> *From:* Ted Ralphs [mailto:ted at lehigh.edu]
> *Sent:* Tuesday, October 28, 2014 2:12 AM
> *To:* Konvicka Filip
> *Cc:* symphony at list.coin-or.org
> *Subject:* Re: [Symphony] Suspicious Code
>
>
>
> 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
>
--
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/20141105/dac83e6b/attachment-0001.html>
More information about the Symphony
mailing list