-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Illegal read in do_is_ordered #236
Comments
I'm not able to replicate this with a simple example. I assumed a zero-length Can you provide more information about the specific > R --quiet -d "valgrind" -e 'xts::isOrdered(integer())'
==3508== Memcheck, a memory error detector
==3508== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==3508== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==3508== Command: /usr/lib/R/bin/exec/R --quiet -e xts::isOrdered(integer())
==3508==
R> xts::isOrdered(integer())
[1] TRUE
R>
R>
==3508==
==3508== HEAP SUMMARY:
==3508== in use at exit: 40,814,488 bytes in 18,669 blocks
==3508== total heap usage: 37,365 allocs, 18,696 frees, 73,276,345 bytes allocated
==3508==
==3508== LEAK SUMMARY:
==3508== definitely lost: 0 bytes in 0 blocks
==3508== indirectly lost: 0 bytes in 0 blocks
==3508== possibly lost: 0 bytes in 0 blocks
==3508== still reachable: 40,814,488 bytes in 18,669 blocks
==3508== suppressed: 0 bytes in 0 blocks
==3508== Rerun with --leak-check=full to see details of leaked memory
==3508==
==3508== For counts of detected and suppressed errors, rerun with: -v
==3508== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) |
I foolishly didn't save the valgrind output when my attempted fix removed the valgrind warning but didn't fix the segfault. I get the same as you for the simple |
I added some debug output here: This verifys that we're in the integer branch and prints out the value of Running this I get:
It looks like it's printing an uninitialised integer but I'm not sure why valgrind isn't complaining. |
I went back to the original case that caused problems and managed to replicate the problem:
I'll run it again with |
Thanks for digging. I'll take a look too. It's not essential to know the cause in order to fix what does look like a bug, but it would be good to know how to trigger the issue in order to write a test. |
Track origins is not particularly illuminating
|
The fix for this should be careful to consider the behavior documented in #221. Any potential changes to user-facing code need to be evaluated carefully. |
Here is a short C function that demonstrates this odd behavior. Also note that it only seems to manifest for me if run via #include <R.h>
#include <Rdefines.h>
#include <R_ext/Error.h>
/* Seem to get random memory if called from R, but not from Rscript. Compare:
R CMD SHLIB weird.c && Rscript -e 'dyn.load("weird.so"); .Call("weird")'
R CMD SHLIB weird.c && R --quiet -e 'dyn.load("weird.so"); .Call("weird")'
*/
SEXP weird(void)
{
SEXP watint = allocVector(INTSXP, 0);
Rprintf("%d\n", INTEGER(watint)[0]);
return R_NilValue;
}
SEXP real_weird(void)
{
SEXP watint = allocVector(REALSXP, 0);
double wat = REAL(watint)[0];
Rprintf("%f\n", wat);
return R_NilValue;
} Assuming the above function is in > R CMD SHLIB weird.c
gcc -std=gnu99 -I/usr/share/R/include -DNDEBUG -fpic -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g -c weird.c -o weird.o
gcc -std=gnu99 -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -Wl,-z,relro -o weird.so weird.o -L/usr/lib/R/lib -lR
> Rscript -e 'dyn.load("weird.so"); .Call("weird"); .Call("real_weird")'
0
NULL
0.000000
NULL
> R --quiet -e 'dyn.load("weird.so"); .Call("weird"); .Call("real_weird")'
R> dyn.load("weird.so"); .Call("weird"); .Call("real_weird")
33817848
NULL
0.000000
NULL
R>
R> |
My first thought was that the code should check for the case where the length is zero and just return true in that case. I think that's what will happen almost surely, unless the uninitialised value is NA_INTEGER, in which case it will return false. Having checked that there is at least one value then accessing x_int[0] should be safe and you prevent any possible segfault (though I don't think I've ever seen a segfault from this) |
@TomAndrews I spent way too much time thinking about this and messing with various attempts at elaborate tests, etc. I think the best solution for now is what you suggested, "check for the case where the length is zero and just return true". Something like this before the if/else branch on if(LENGTH(x) < 1) {
return ScalarLogical(1);
} I would prefer you make the change and submit a PR, since you found this and provided the solution. I would like you to get the credit. Or I can make the change; let me know your preference. |
do_is_ordered tries to check whether the first element of the series is NA. It needs to first check whether the series has at least one element to avoid reading an uninitialised value. Fixes joshuaulrich#236.
Thanks! I've made a pull request here #261 |
Description
In the process of trying to track down the cause of #234 I ran some jobs through valgrind and it highlighted a jump based on an uninitialized value here:
https://github.com/joshuaulrich/xts/blob/master/src/isOrdered.c#L80
I don't think
do_is_ordered
checks that the length ofx
is at least 1 before checking whether the first value isNA
.I guess there could be a similar problem here:
https://github.com/joshuaulrich/xts/blob/master/src/isOrdered.c#L107
I think
nx
could be-1
soint_x[nx]
may be an illegal read.The text was updated successfully, but these errors were encountered: