CODEWITHSUNDEEP
c++
Cyths
Cyths

Reputation: 3

Pointer type casting altering unintended memory

#define ARRAY_SIZE 20

float DataSource[ARRAY_SIZE];

void Read(unsigned char const *Source, unsigned char *Destination, unsigned long DataSize)
{
  for ( unsigned long i = 0; i < DataSize; i++)
  {
    *(Destination + i*DataSize) = *(Source + i*DataSize);
  }
}

void fun()
{
  int Index;
  float Dest;

  for ( Index = 0; Index < ARRAY_SIZE; Index++ )
  {
    Read((unsigned char *)&DataSource[Index], (unsigned char *)&Dest, sizeof(DataSource[Index]));
  }
}

I'm having an issue with the above code where upon calling Read(), my Index variable gets overwritten and I am certain the ugly pointer casting is the culprit, but I'm having trouble understanding exactly what is happening here.

The unsigned char pointer types are mandatory because the above code is intended to simulate some driver level software and maintain the same prototype.

Can someone help me to understand the issue here? All the above code is changeable except for the prototype of Read().

Upvotes: 0

Views: 149

Answers (6)

Christoph Freundl
Christoph Freundl

Reputation: 627

You are treating the scalar variable Dest declared inside fun() as an array inside Read(). It seems that both Dest and your Index variable are placed adjacent on the stack which explains that Index gets overwritten exactly when the loop inside Read() is executed for i==1.

So the solution is: declare Dest as an array, too:

float Dest[ARRAY_SIZE];

Upvotes: 0

Yakk - Adam Nevraumont
Yakk - Adam Nevraumont

Reputation: 275565

An amusing (to me) C++ way.

template<typename Data>
struct MemBlockRefHelper {
  typedef Data value_type;
  Data* data;
  size_t size;
  MemBlockRefHelper( Data* d, size_t s):data(d), size(s) {}

  template<typename Target, typename Other=typename Target::value_type>
  Target& Assign( MemBlockRefHelper<Other> const& other ) {
    Assert(size == other.size);
    for (size_t i = 0; i < size; ++i) {
      if (i < other.size) {
        data[i] = other.data[i];
      } else {
        data[i] = 0;
      }
    }
    Target* self = static_cast<Target*>(this);
    return *self;
  }

};
struct MemBlockRef;
struct MemBlockCRef:MemBlockRefHelper<const unsigned char> {
  MemBlockCRef( const unsigned char* d, size_t s ):MemBlockRefHelper<const unsigned char>( d, s ) {}
  MemBlockCRef( const MemBlockRef& other );
};
struct MemBlockRef:MemBlockRefHelper<unsigned char> {
  MemBlockRef( unsigned char* d, size_t s ):MemBlockRefHelper<unsigned char>( d, s ) {}
  MemBlockRef& operator=( MemBlockRef const& other ) {
    return Assign< MemBlockRef >( other );
  }
  MemBlockRef& operator=( MemBlockCRef const& other ) {
    return Assign< MemBlockRef, const unsigned char >( other );
  }
};
inline MemBlockCRef::MemBlockCRef( const MemBlockRef& other ): MemBlockRefHelper<const unsigned char>( other.data, other.size ) {}

void Read( unsigned char const* Source, unsigned char* Dest, unsigned long DataSize ) {
  MemBlockCRef src( Source, DataSize );
  MemBlockRef dest( Dest, DataSize );
  dest = src;
}

massively over engineered, but the idea is to wrap up the idea of a block of POD memory of a certain size, and provide reference semantics to its contents (initialization is creating a new reference to the same data, assignment does a copy over the referred to data).

Once you have such classes, the code for Read becomes a 3 liner. Well, you can do it in one:

  MemBlockRef( Dest, DataSize ) = MemBlockCRef( Source, DataSize );

but that is needless.

Well, so it this entire framework.

But I was amused by writing it.

Upvotes: 1

Anton Kovalenko
Anton Kovalenko

Reputation: 21507

Let's take a closer look at your Read(): i changes from 0 to DataSize-1; each time you access memory by an offset of i*DataSize... that is, by an offset from 0 to DataSize*(DataSize-1). Looks wrong, as DataSize**2-DataSize makes no sense.

Unlike other answers, I don't want to guess what you wanted. Just showing a kind of "dimensional analysis" that can help spotting the wrongest part of code without reading the author's mind.

Upvotes: 0

Ben Voigt
Ben Voigt

Reputation: 283684

This is wrong:

*(Destination + i*DataSize) = *(Source + i*DataSize);

You want to copy DataSize adjacent bytes, not bytes DataSize apart (total span DataSize*DataSize)

Just say

Destination[i] = Source[i];

Upvotes: 1

Mark B
Mark B

Reputation: 96261

You pass in a single float's address to Read (&Dest) and then proceed to write many valuese to consecutive memory locations. Since you're writing random memory at that point it's not unlikely that it could have overwritten index (and other stuff) because stacks usually grow downwards.

Upvotes: 2

Synxis
Synxis

Reputation: 9388

The error is here:

for ( unsigned long i = 0; i < DataSize; i++)
{
  //              vvvvvvvvvv               vvvvvvvvvv
  *(Destination + i*DataSize) = *(Source + i*DataSize);
}

i * DataSize is always greater than i => "out of bound" access.

Replace with:

for ( unsigned long i = 0; i < DataSize; i++)
{
  *(Destination + i) = *(Source + i);
}

Upvotes: 4

Related Questions