encryptogroup / ABY

ABY - A Framework for Efficient Mixed-protocol Secure Two-party Computation
GNU Lesser General Public License v3.0
463 stars 132 forks source link

Possible problems with operator precedence #18

Closed lenerd closed 7 years ago

lenerd commented 7 years ago

There are a few comparisons in combination with bitwise operators. Since these operators have lower precedence than the e.g. ==, the code may not do what was indended by the author

I suggest to place parentheses even if the expression is evaluated as intended.

The line numbers are a bit off, but I linked to the correct lines in this repository.


here

src/abycore/sharing/yaoserversharing.cpp:827:53: warning: ^ has lower precedence than ==; == will be evaluated first [-Wparentheses]
                                        if (m_vClientROTRcvBuf.GetBitNoMask(linbitctr) ^ permval == 1) {
                                                                                       ^~~~~~~~~~~~~~
src/abycore/sharing/yaoserversharing.cpp:827:53: note: place parentheses around the '==' expression to silence this warning
                                        if (m_vClientROTRcvBuf.GetBitNoMask(linbitctr) ^ permval == 1) {
                                                                                       ^
                                                                                         (           )
src/abycore/sharing/yaoserversharing.cpp:827:53: note: place parentheses around the ^ expression to evaluate it first
                                        if (m_vClientROTRcvBuf.GetBitNoMask(linbitctr) ^ permval == 1) {
                                                                                       ^
                                            (                                                   )

here

src/abycore/util/cbitvector.cpp:198:47: warning: operator '<<' has lower precedence than '-'; '-' will be evaluated first [-Wshift-op-parentheses]
                        p[i] = ((m_pBits[posctr] & (((1 << remlen) - 1 << lowermask))) >> lowermask) & 0xFF;
                                                     ~~~~~~~~~~~~~~^~~ ~~
src/abycore/util/cbitvector.cpp:198:47: note: place parentheses around the '-' expression to silence this warning
                        p[i] = ((m_pBits[posctr] & (((1 << remlen) - 1 << lowermask))) >> lowermask) & 0xFF;
                                                                   ^
                                                     (                )

here

src/examples/psi_phasing/common/phasing_circuit.cpp:287:9: warning: & has lower precedence than >; > will be
evaluated first [-Wparentheses]
                        if(j & 0x01 > 0) { //value is odd, hence store highest value on stash
                             ^~~~~~~~~~
src/examples/psi_phasing/common/phasing_circuit.cpp:287:9: note: place parentheses around the '>' expression to silence this warning
                        if(j & 0x01 > 0) { //value is odd, hence store highest value on stash
                             ^
                               (       )
src/examples/psi_phasing/common/phasing_circuit.cpp:287:9: note: place parentheses around the & expression to evaluate it first
                        if(j & 0x01 > 0) { //value is odd, hence store highest value on stash
                             ^
                           (       )

here

src/test/abytest.cpp:177:17: warning: ^ has lower precedence than ==; == will be evaluated first [-Wparentheses]
                                verify = sa ^ sb == 0 ? b : a;
                                            ^~~~~~~~~
src/test/abytest.cpp:177:17: note: place parentheses around the '==' expression to silence this warning
                                verify = sa ^ sb == 0 ? b : a;
                                            ^
                                              (      )
abytest.cpp:177:17: note: place parentheses around the ^ expression to evaluate it first
                                verify = sa ^ sb == 0 ? b : a;
                                            ^
                                         (      )
dd23 commented 7 years ago

Thanks for reporting this issue. The 4 cases have been fixed in the recent merge from master in f57e24669e445d2c19924dcd5069aa13f4a64da4