[Kde-games-devel] Review Request 126919: Fix crash when placing bonuses in the arena

[Mathias Kraus]

> On Jan. 29, 2016, 11:16 a.m., Frederik Schwarzer wrote:
> > In line 354 of the same file the author already tries to handle the 4th quarter.
> > ```cpp
> >     nQuarterSize = (nQuarter < 3 ? nFullSize / 4 : nFullSize - 3 * nFullSize / 4);
> > ```
> > Can you fix the issue in that line instead?
> > I mean, if I divide by 4, I am always at the lower bound of possibilities. E.g. with a size of 75, I get 18.75 mathematically, which is 18 with the '/'.
> > If I then take three fourth of the size (size*3/4), it's 56,25, resulting in 56 being subtracted from the 75, which is 19. Boom?
> > 
> > So I would suggest something like:
> > ```cpp
> > -        nQuarterSize = (nQuarter < 3 ? nFullSize / 4 : nFullSize - 3 * nFullSize / 4);
> > +        if (nQuarter < 3) {
> > +            nQuarterSize = nFullSize / 4;
> > +        } else {
> > +            if ((nFullSize * 3 % 4) == 0) {
> > +                nQuarterSize = nFullSize - nFullSize * 3 / 4;
> > +            } else {
> > +                nQuarterSize = nFullSize - nFullSize * 3 / 4 - 1;
> > +            }
> > +        }
> > ```
> > around line 354 so nQuarterSize is correct throughout the whole rest of that method.
> > 
> > Or am I barking up the wrong tree here?
> 
> Mathias Kraus wrote:
>     This would also fix the crash, but the solution from Julian is how it was supposed to work. Because of the division by 4, the sum of the four quarters would be less than nFullSize. That was the reason for the special handling of the fourth quarter, which will be equal or bigger than the privious three. But the index of the fourth quarter should start at the end of the third quarter, not three times the possibly bigger fourth quarter, so the solution from Julian is correct.
>     
>     The solution from above will set the bonus to a block already set in quarter 3. Let's assume nFullSize is a multiple of 4, e.g. 16. In this case the last quarter would have a size of 3. The start index would be calculated to 9, which would be part of the third quarter and could overwrite an already set bonus from m_blocks.
>     
>     The calculation of the fourth quarter is also not correct, it should be
>     ```cpp
>     nQuarterSize = (nQuarter < 3 ? nFullSize / 4 : nFullSize - 3 * (nFullSize / 4));
>     ```
>     else the last quarter will be to small. Julian, could you also fix tha and then commit?

BTW, thank's Julian for looking into the problem :)


- Mathias


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126919/#review91767
-----------------------------------------------------------


On Jan. 28, 2016, 1:30 p.m., Julian Helfferich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126919/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2016, 1:30 p.m.)
> 
> 
> Review request for KDE Games.
> 
> 
> Repository: granatier
> 
> 
> Description
> -------
> 
> The crash takes place in game.cpp - Game::createBonus(). The total amount of blocks is nFullSize. To place the bonuses, the game iterates over 4 quarters, the first three containing nFullSize/4 blocks and the last containing the remaining blocks. Thus, nQuarterSize can be larger for the last quarter than for the previous three. Now, when a bonus is assigned to a block, the index of the block is calculated as
> 
> nIndex = nQuarter * nQuarterSize + i
> 
> where i iterates from zero to nQuarterSize. The idea is that nQuarter * nQuarterSize is the number of blocks of the previous quarters. However, since nQuarterSize can be larger for the last quarter, this can lead to an index out of bounds when a bonus is to be placed in one of the last blocks. The fixed version is
> 
> nIndex = nQuarter * (nFullSize/4) + i
> 
> where nFullSize/4 is the size of the first three quarters.
> 
> 
> Diffs
> -----
> 
>   src/game.cpp 371fac9 
> 
> Diff: https://git.reviewboard.kde.org/r/126919/diff/
> 
> 
> Testing
> -------
> 
> Started the game a lot of times. Crash did not happen.
> 
> 
> Thanks,
> 
> Julian Helfferich
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-games-devel/attachments/20160129/7fbb144c/attachment-0001.html>