Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix illegal access when there are more orientations than allocated #150

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mitjap
Copy link
Contributor

@mitjap mitjap commented Jul 5, 2023

This also resolves #105.

I am also attaching a sample image where this error happens. 1.pgm.txt

Description

This PR resolves an issue with illegal memory access when there are more orientations/descriptors than allocated. This error happens when there at least 2 times more orientations than there are extremas. There must also be at least 100k extremas for this crash to happen. You can reduce this value to get this error easier.

, _max_extrema( 100000 )

I'm pasting some link for easier understanding which parts are relevant for memory allocation and its amount.

h_consts.max_extrema = max_extrema;
h_consts.max_orientations = max_extrema + max_extrema/4;

sz = max( 2 * h_consts.max_extrema, h_consts.max_orientations );

numExtrema *= 2;

This part here is interesting because there is a constant max_orientations = max_extrema + max_extrema/4; but the amount allocated is then max( 2 * h_consts.max_extrema, h_consts.max_orientations ) which does not make much sense. But that in itself is not a problem. The problem is that excess orientations are never pruned which causes problem later in the process. This makes it mandatory for checking if orientation index is within allocated memory.

Implementation remarks

Instead of pruning excess orientations I've implemented index checking at relevant places. Pruning might be better solution but it is not as easy to implement (maybe I am wrong). If pruning was to happen I think it should be done here:

ExtremaWrt w( extremum );
ExtremaTot t( total_ori );
ExtremaWrtMap wrtm( feat_to_ext_map, max( d_consts.max_orientations, dbuf.ori_allocated ) );
ExclusivePrefixSum::Block<ExtremaRead,ExtremaWrt,ExtremaTot,ExtremaWrtMap>( total_ext_ct, r, w, t, wrtm );

@mitjap mitjap marked this pull request as draft July 5, 2023 10:11
@mitjap mitjap changed the title fix illegal access when there are more descriptors than allocated fix illegal access when there are more orientations than allocated Jul 5, 2023
@mitjap mitjap marked this pull request as ready for review July 5, 2023 10:29
@griwodz griwodz self-assigned this Jul 26, 2024
@griwodz
Copy link
Member

griwodz commented Jul 29, 2024

Instead of changing all the descriptor extraction types, I would prefer to fix the out-of-bounds problem in s_orientation.cu as you propose.

But instead of changing ExclusivePrefixSum::Block, wouldn't it be better to fix the loop

        for( int o=1; o<MAX_OCTAVES; o++ ) {
            dct.ori_ps[o] = dct.ori_ps[o-1] + dct.ori_ct[o-1];
        }

by adding a max( ..., dbuf.ori_allocated-1 ) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

run image crash [bug]
3 participants