#211 Hidden danger unsigned integer

[Alexey Kostin]

operator[](unsigned int) has potential danger
for example in
template <class DataT> inline
bool BufferAccessIterRC<DataT>::Last(const
RangeBufferAccessC<DataT> &buff) {
if(buff.Size() <= 0) {
at = 0;
endOfRow = 0;
return false;
}
at = const_cast<DataT *>(&buff[buff.IMax()]);
endOfRow = &(at[-buff.Size()]); //very bad for 64 bit
return true;
}
buff.Size() is unsigned int say 0xa
operator- makes it 0xfffffff6
after it going addition of the value to pointer. In 32
bit architecture this gives expected result due to
losing overflowing bits, but in 64 bit there is anough
bits to hold full result, which causes wrong result.
This is my test
unsigned int s = 10;
int* arr = new int[s];
int* eptr = arr+s;
int* bptr = eptr-s; //correct result subtraction uint
from poiter
int* eptr1 = &(arr[s]);
int* bptr1 = &(eptr1[-s]); //incorrect operation
unary minus
if(bptr != bptr1)
cout << "error\n";
Any idea how to fix it? I presume there are lots of
places in RAVL having the hidden danger.

Alex

[Alexey Kostin]

Debugger screen snapshot

[Charles Galambos]

Logged In: YES
user_id=188615

Eeek! Nasty.

I don't think this construction is common at all and I think
a fix should be fairly simple. Will look into it now

[Charles Galambos]

Logged In: YES
user_id=188615

This has been fixed and commited to CVS.

[Charles Galambos]

Logged In: YES
user_id=188615

I've checked in a fix for this using the sutraction
operator, (As I remember, I didn't use this construction at
the time as there were some compiler bugs which gave
incorrect results, now long fixed. )

It doesn't look like RAVL's been correctly configured, if
your working in a 64 bit address space SizeT should be
defined as a 64 bit number.

Secondly if you define s as something like an unsigned char
the code works correctly which leads me to think it might
also be a compiler bug.

[Alexey Kostin]

Logged In: YES
user_id=827933

It very common if you include here constructions like
template<class InPixelT,class OutPixelT,class KernelPixelT,
class SumTypeT>
void ConvolveVert2dC<InPixelT,OutPixelT,KernelPixelT,
SumTypeT>::Apply(const ImageC<InPixelT> &in,
ImageC<OutPixelT> &result) const {
....
UIntT ksize = colKernel.Range().Size();
UIntT rowSep = in.Stride();
for(Array2dIter2C<OutPixelT,InPixelT> it(result,in,resRect);it;
it++) { // ,resRect
const KernelPixelT *kp = &(colKernel[colKernel.IMin()]);
const KernelPixelT *eol = &kp[ksize];
const InPixelT *ip = &((&it.Data2())[colKernel.IMin().V() *
rowSep]); //IntT * UIntT result is UIntT!!!

So, in short, bug is in improper use of UIntT. According to C
standard unsigned integer is always positive number. In RAVL
it is not always true. Like in the examples above.
The problem apears when we do type conversion from 32 to 64
bit. The processor just padds top 32 bits of original 'unsigned
int' with zeros then converted to 64 bits version, which is
correct behaviour according to C standard. But this is not what
expected in the RAVL functions.

Alex

[Charles Galambos]

Logged In: YES
user_id=188615

I would completely agree with your statment about unsigned
integer and sign extending bits. If there are places in
RAVL where an unsigned integer is being used as a sign
valued as in the code you shown, its a bug and should be
fixed. Please let me know of all the places its an issue,
I'll start reviewing code for similar problems.

[Alexey Kostin]

Logged In: YES
user_id=827933

I think I will just start correcting code myself then.