fixing "invalid permission" segfault from #5077 by only starting the amount of thread, memory was allocated for.

[bennet0496]

Closes #5077

After first finding that the allocated amount of memory for the TMP buffer seamed too low, 0x13c0000 vs 80*UINT_16_MAX*sizeof(int) = 0x13ffec0 and was followed by a protected 0-page with setDTthreads(threads = 80), it turned out that in L717-L718 the allocated memory was actually only 79*UINT_16_MAX*sizeof(int) = 0x13bfec4, because getDTthreads was called with throttle = true.

But later when starting the parallel threads, in L1235-L1238 the number of threads to start was determined with throttle = false, causing the segfault at L995 for threads over the throttled threshold, while trying to access memory past the TMP buffer.

Starting only the throttled amount of threads, fixes that. This also seamed fixed the random double free or corruption errors as they are currently not occurring anymore for 80 threads like before.

Unfortunately, it seems to be quite difficult to find a reasonably small dataset and requirement in thread count to write a test for, as quite some things need to happen, that this error occurs. The requirement I currently found are

• the dataset must be groupable into enough groups, that parallel execution will occur (L1195-L1243)
• groups need to have the correct size (<65535?) for the parallel region L1235-L1239 to be run
• groups or slices of the group (?) must be larger than 256 and smaller than 65535 for L936-L1036 (containing the critical L995) to run
• the number of configured threads to run need to be larger than the number of throttled threads (the ceiling of the number of groups divided by 1024 (R_DATATABLE_THROTTLE)), that less memory is allocated than used
• the group for the thread with a thread-number higher than the ceiling of the number of groups divided by 1024 must have enough elements for the out of bound access to happen on the next protected page.
• the number of throttled threads needs to be large enough, that the malloc() in L718 will internally use mmap() instead of the default brk()/sbrk() so that the protected page is not moved by new heap allocations

In other words it seems to be a very specific edge case and the scientist that created the dataset that causes the crash, should maybe start to gamble :)

[mattdowle]

Thanks for all this great detail. LOL at the last sentence.
I just had a quick look at the proposed fix so far, and I can see why it works by applying the throttle which reduces the number of threads. But false seems like the correct value there since the loop is through ngrp:

#pragma omp parallel for schedule(dynamic) num_threads(getDTthreads(ngrp, false))
for (int i=0; i<ngrp; i++) {

The comments in getDTthreads for throttle are:

// throttle==true  : a number of iterations per thread (DTthrottle) is applied before a second thread is utilized
// throttle==false : parallel region is already pre-chunked such as in fread; e.g. two batches intended for two threads

I'm thinking it's the true on L717 that needs to be false?

[mattdowle]

This grep finds this one, and 4 others to review: all getDTthreads(..., true) calls not on #pragma lines :

$ grep -n "getDTthreads(.*true" *.c --exclude=openmp-utils.c --exclude=init.c | grep -v pragma 
forder.c:717:  nth = getDTthreads(nrow, true);  // this nth is relied on in cleanup()
forder.c:1039:  int batchSize = MIN(UINT16_MAX, 1+my_n/getDTthreads(my_n, true));  // (my_n-1)/nBatch + 1;   //UINT16_MAX == 65535
fsort.c:116:  int nth = getDTthreads(xlength(x), true);
gsumm.c:82:  nBatch = MIN((nrow+1)/2, getDTthreads(nrow, true)*2);  // *2 to reduce last-thread-home. TODO: experiment. The higher this is though, the bigger is counts[]
subset.c:14:  int nth = getDTthreads(n, /*throttle=*/true);   // not const for Solaris, #4638

I was thinking that all getDTthreads() calls not on #pragma lines should be enforced to be false; i.e. the 5 calls above. But then, for a large number of threads such as the 80 in issue #5077, working memory would be allocated for the 80 threads which would be wasteful when throttling.

Maybe the problem in general is that we call getDTthreads() again on #pragma lines, where we should instead use nth where there is one, to ensure the throttle=true|false choice matches the allocation.

[bennet0496]

LOL at the last sentence.

I mean, from what I've seen, it looks pretty unlikely to me to end up with a dataset fulfilling all the requirement to hit this error. And there is a saying where I'm from, that when you manage to do something very unlikely by pure chance, than you have enough luck to gamble or more specifically to play the lottery (even though mathematically this would make no sense). But that's besides the point :)

But false seems like the correct value there since the loop is through ngrp
[...]
I'm thinking it's the true on L717 that needs to be false?

To be honest, I'm not completely sure which would be the correct value in either place. The code is quite hard to read, and it took me a good week to understand it enough to find this error. Also throttling the parallel region, to fix the problem, seamed least intrusive to me, as throttling seamed to be applied for a good reason in the first place.

I was thinking that all getDTthreads() calls not on #pragma lines should be enforced to be false; i.e. the 5 calls above. But then, for a large number of threads such as the 80 in issue #5077, working memory would be allocated for the 80 threads which would be wasteful when throttling.

Fair enough, allocating the upper bound of what could possibly be used will definitely also solve the problem. It looks like, the overhead would be about 256kB per additional thread, which is arguably not optimal but probably also not too bad.

Maybe the problem in general is that we call getDTthreads() again on #pragma lines, where we should instead use nth where there is one, to ensure the throttle=true|false choice matches the allocation.

I agree, that it would probably be best to reuse the value where possible and not calculate it again to prevent mismatching option and therefore calculations. I will take a look at that. Maybe using the minimum of nth and the calculated value in the pragma lines would also be an option, as the basis on which the values are calculated seam to be different. I case at some point it desirable to start even less threads than anticipated during memory calculation.

[bennet0496]

Okay so I traced back the values and found, that

now for the first call of radix_r(), directly after the memory allocation, my_n = nrow. This means for the blue box (with a sufficiently large number of configured threads)

And for the yellow box

While during allocation of TMP with a sufficiently large number of configured threads nth is calculated as

So it looks like usually one more thread is started than memory was allocated for. So when the additional conditions to reach L995 are met, the buffer overflow occurs.

It also looks like this global TMP is used by the code around L995, so it seems to overhead in some (or most?) of the time anyway, so it probably also wouldn't hurt to allocate the additional 256kB per thread for this buffer to be safe that no overflow occurs, instead of throttling the number of threads to start.

[mattdowle]

Have invited you to be project member so among other things you can create branches in the main project in future. The invite should be a button you need to click to accept in GitHub in either your projects or profile page. Have also added you to the contributors list. Thanks again!

[mattdowle]

Calling getDTthreads() again here still leaves the same door open that this value of throttle needs to be the same as the value used to allocate on line 718. We can use nth on the #pragma line, avoid the new variable, avoid the extra call to getDTthreads() and reduce from 3 places to 2 places. Will do...

[bennet0496]

I don't think calling it again here with the MIN limiting it to the value of nth is necessarily the same problem. But thinking about it again and seeing the last commits you added, I'd say it was redundant, as getDTthreads w/ throttle=false, basically only limits to the number of configured threads, which is already done w/ nth. So if configured threads are the limiting factor, this already resembled in nth so there is no need to apply it again to ngrp as it can then only play a role here if ngrp < threads, but then also ngrp < nth would be true. So doing like you did with MIN(nth, grp) is more efficient as it saves the function call.

[bennet0496]

I renamed the variable, because it made it easier to distinguish from the other block-local TMP(s?) during debugging. But yes, for consistency I probably should have named it back, after I was done.

[bennet0496]

This was the wrong of the two comment buttons, I guess. I'm sorry

[bennet0496]

When mentioning the processor used, I think it would be better to use 128 threads as the example, as this would be the default value on such a machine. The other values mentioned (such as 80 or 96) were manually configured to initially find the error threshold. And I don't know how common it is for people to manually set the amount of threads to be used.

[mattdowle]

Thanks, I see. I'm going to try and create a test which reliably fails under ASAN (the commands are in CRAN_Release.cmd). I suspect it's not failing very often currently because there's space after the end of TMP within the last page that's effectively ok to write to (or something like that). Turning on ASAN should trap that in more cases given a suitable test. I never like using the words "in some circumstances" in the news items because I sense the reader thinking "well, what circumstances, and might my circumstances be affected?". So once the test is ready I'll go back and replace that phrase and the number of threads and cores with more detail.

[bennet0496]

Yes with ASAN it should not be too hard to find a reliably failing example. The examples I have on my work PC might even produce the error with this, but I will not be able to access them until tomorrow noon (CEST time). But I think even this example required at least 6 threads or something like that, and I don't know what is possible in the test environment or what the limits are.

And I wouldn't call it "effectively ok to write to" but "possible to write to, without the environment noticing", as buffer overflows are always inherently problematic, as it may overwrites other data for other program parts or the overflown data is unexpectedly overwritten by other parts of the program, especially if the data is small enough for heap allocation to occur.

[mattdowle]

Yes that is better wording. I do know that.

[mattdowle]

This is enough to segfault and doesn't need ASAN:

DT = data.table(grp=sample(65536))  # >=65536 necessary
setDTthreads(throttle=nrow(DT))     # increase throttle to reduce threads to 1 for this nrow
DT[, .N, by=grp]                    # segfault
setkey(DT, grp)                     # segfault

[bennet0496]

Oh ok, I didn't know (or find) the throttle value is adjustable like this. I tried to find an example with the default 1024 value, which made it much harder, to find a small example that would even remotely reach the protected page.