D15716: Suspicious use of signed arithmetic leads to negative index crash

[Jonathan Marten]

marten created this revision.
marten added a reviewer: KDE Games.
Herald added a subscriber: kde-games-devel.
marten requested review of this revision.

REVISION SUMMARY
  The card deck is shuffled in shuffled() (in dealer.cpp) using a random number generator that, according to the comments above, is identical to the Freecell one.  I'm not familiar with the details of the Windows implementation, but the kpat implementation uses arithmetic operators on signed values which according to the C11 standard (quoted at https://wiki.sei.cmu.edu/confluence/display/c/INT13-C.+Use+bitwise+operators+only+on+unsigned+operands) can give implementation defined results.
  
  What I have observed when recompiling kpat recently (possibly after a switch to GCC 7.3.0) is the 'seed' value after a few iterations of the loop becoming negative due to integer overflow.  Then 'rand' also becomes negative (despite the '& 0x7fff'!), 'rand % i' again is negative and the result.swap() below segfaults.
  
  Changing the type of the 'seed' variable to unsigned appears to avoid the problem, although I don't know whether this will be incompatible with the Freecell implementation.

TEST PLAN
  Run kpat without this change, select and deal a new game.  A segfault will regularly occur at the result.swap() line.
  Build and run kpat with this change, there is no segfault and the game is dealt correctly.

REPOSITORY
  R410 KPatience

REVISION DETAIL
  https://phabricator.kde.org/D15716

AFFECTED FILES
  dealer.cpp

To: marten, #kde_games
Cc: kde-games-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-games-devel/attachments/20180923/24cf06c6/attachment.html>