Skip to content

[FIX] relief: Fix contingency (de)allocation#4745

Merged
janezd merged 1 commit intobiolab:masterfrom
ales-erjavec:fixes/relief-use-after-free
May 8, 2020
Merged

[FIX] relief: Fix contingency (de)allocation#4745
janezd merged 1 commit intobiolab:masterfrom
ales-erjavec:fixes/relief-use-after-free

Conversation

@ales-erjavec
Copy link
Contributor

@ales-erjavec ales-erjavec commented May 7, 2020

Issue

_contingency_table in relief.pyx does not retain np.ndarray allocated data buffer after function returns even though it 'inserts' a cython MemoryView into a c++ std::map, which is then used later (I think MemoryView cannot be readily inserted in c++ std containers and preserve referential integrity).

This can lead to a segfault or memory corruption. It can be triggered on macOS with

MallocScribble=1 PYTHONMALLOC=debug python -X dev -m unittest -v -b  Orange.widgets.data.tests.test_owrank.TestOWRank.test_dataset{,,,,}
Description of changes

Use Dict[int, np.ndarray] instead of std::map[ssize_t, MemoryView] as 'contingencies'. Acquire GIL when necessary when accessing elements.

Includes
  • Code changes
  • Tests
  • Documentation

@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #4745 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4745      +/-   ##
==========================================
+ Coverage   83.69%   83.74%   +0.05%     
==========================================
  Files         275      275              
  Lines       55620    55655      +35     
==========================================
+ Hits        46551    46611      +60     
+ Misses       9069     9044      -25     

@janezd janezd self-assigned this May 7, 2020
@janezd janezd merged commit 2a56770 into biolab:master May 8, 2020
@ales-erjavec ales-erjavec deleted the fixes/relief-use-after-free branch August 5, 2020 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants