FOV memory issue

[Harms]

Hey!
I am (as of not long ago) the package maintainer of tome4 in AUR.
I know the language C quite well, and I have noticed a bug in fov/fov.c.
It caused my compilation of tome4 to crash every time as the game was attempting to render the background of the main menu.

Putting a watch on global_shape showed that this variable was written to inside LARGE_ASS_FOV_DEFINE_OCTANT, and further investigations lead me to line 599 and GET_BUFFER:

Code: Select all

578        #define GET_BUFFER(target, buffer_data, len)                                                      \
579            /* hurray, no branching, modulus, or malloc! */                                               \
580            buffer_data->index = (buffer_data->index + buffer_data->prev_len) & (FOV_BUFFER_SIZE - 1);    \                                                                                             \
581            buffer_data->prev_len = len;                                                                  \
582            target = buffer_data->buffer + buffer_data->index;

This bit of code is for allocating small buffers at high intervals (I guess), where buffer_data is the preallocated pool of memory.
Problem is that if buffer_data->index after line 580 ends up near the end of the buffer in such a way that the requested chunk of memory can't fit within the pool's boundaries it will usually cause a segmentation fault (or the infamous general protection fault from XP days).

We need a check here to make sure that the requested chunk is placed correctly within the pool (buffer_data); here is my suggestion:

Code: Select all

578        #define GET_BUFFER(target, buffer_data, len)                                                      \
579            /* hurray, no branching, modulus, or malloc! */                                               \
580            buffer_data->index = (buffer_data->index + buffer_data->prev_len) & (FOV_BUFFER_SIZE - 1);    \
581            if (buffer_data->index+len >= FOV_BUFFER_SIZE) {                                              \
582                buffer_data->index = 0;                                                                   \
583            }                                                                                             \
584            buffer_data->prev_len = len;                                                                  \
585            target = buffer_data->buffer + buffer_data->index;

Unfortunately we must also remove the 'hurray', because I cannot see how we may avoid branching here
Luckily most of the time the if will fail, and modern CPU's exploit this and are able to preload assembly as if the if will always fail. See http://en.wikipedia.org/wiki/Branch_predictor.
The bottom line is that this branch should have absolutely no impact on performance

I have fixed this in the AUR package with a homebrew patch, so please tell me if you implement this fix and I must update the package

BR Thomas