Issues in generating unique time id using virtual memory address

[ittium]

Group,
I came across a timer implementation where to generate unique timer id,
implementer has used virtual memory address. In the program written
below, using new, 1 byte memory is allocated, this memory is kept
reserved till timer expiry. Since this address will be unique (in
program virtual memory), uniqueness of timer id is guaranteed. This code
is used in a multi-threaded program and is supposed to generate unique
ids on 32bit and 64bit machines. The code always work fine during unit
testing. But in field trials (code running for more than a month or so)
, some time, we have seen on a 32 bit machine code generates junk
timerId. I doubt a possible heap corruption, but we can not run memory
profilers in field trials. Please go through the code. I have added my
comments, where I think code might have some issue.

#include <iostream>
#include <stdlib.h>

using namespace std;

typedef uint64_t BYTE8;
typedef unsigned char BYTE;

#define _AAANEW(ptr,structClass) \
try \
{ \
ptr = NULL; // Comment - not needed but see the catch block
ptr = new structClass; \
} \
catch(...) \
{ \
if(ptr == NULL) \
{ \
cout<<"Failure in _AAANEW, exiting the process";\
exit(0); \
} \
}

BYTE8 getTimerId()
{


BYTE* timer;
_AAANEW(timer,BYTE); //Comment - On 32 bit machine, timer will
//contain 32 bit virtual memory address
/* Timer id is taken as BYTE 8 since timer Id will be generated
on 32 bit an 64 bit also */
BYTE8 timerId = (BYTE8)timer; //Comment - I am not sure
// whether for 32 bit machine this
//conversion is safe
return timerId;
}
thanks
Ittium

 

[Jorgen Grahn]

Group,
I came across a timer implementation where to generate unique timer id,
implementer has used virtual memory address. In the program written
below, using new, 1 byte memory is allocated, this memory is kept
reserved till timer expiry. Since this address will be unique (in
program virtual memory), uniqueness of timer id is guaranteed.

You can skip all the talk about virtual memory. C++ doesn't care
about that. It simply guarantees that

T* const a = new T();
T* const b = new T();
assert(a!=b);

for as long as *a and *b exist.

This code
is used in a multi-threaded program and is supposed to generate unique
ids on 32bit and 64bit machines. The code always work fine during unit
testing. But in field trials (code running for more than a month or so)
, some time, we have seen on a 32 bit machine code generates junk
timerId. I doubt a possible heap corruption, but we can not run memory
profilers in field trials. Please go through the code. I have added my
comments, where I think code might have some issue.

#include <iostream>
#include <stdlib.h>

using namespace std;

typedef uint64_t BYTE8;

What a weird typedef! What's wrong with uint64_t?

typedef unsigned char BYTE;

#define _AAANEW(ptr,structClass) \
try \
{ \
ptr = NULL; // Comment - not needed but see the catch block
ptr = new structClass; \
} \
catch(...) \
{ \
if(ptr == NULL) \
{ \
cout<<"Failure in _AAANEW, exiting the process";\
exit(0); \
} \
}

Ugh. I won't comment on that ...

BYTE8 getTimerId()
{


BYTE* timer;
_AAANEW(timer,BYTE); //Comment - On 32 bit machine, timer will
//contain 32 bit virtual memory address
/* Timer id is taken as BYTE 8 since timer Id will be generated
on 32 bit an 64 bit also */
BYTE8 timerId = (BYTE8)timer; //Comment - I am not sure
// whether for 32 bit machine this
//conversion is safe
return timerId;
}

OK, so yo allocate an ID by new:ing a small object, cast its address
to uint64_t, and that's the ID. Presumably you never delete that
small object? I see nothing wrong with it on systems with flat
addresses less than or equal 64-bits. I think your bug is elsewhere.

By the way, why don't you just say "the timer ID is an unsigned
char*"? Why bother to squeeze it into an integer?

/Jorgen

 

[Pavel]

Group,
I came across a timer implementation where to generate unique timer id,
implementer has used virtual memory address. In the program written below,
using new, 1 byte memory is allocated, this memory is kept reserved till timer
expiry. Since this address will be unique (in program virtual memory),
uniqueness of timer id is guaranteed. This code is used in a multi-threaded
program and is supposed to generate unique ids on 32bit and 64bit machines. The
code always work fine during unit testing. But in field trials (code running for
more than a month or so) , some time, we have seen on a 32 bit machine code
generates junk timerId.

How do you determine when the timerId is junk?

Are you trying to narrow that BYTE8 back to the pointer type and are not finding
the original BYTE8* there? How are you doing the reverse conversion?

I doubt a possible heap corruption, but we can not run
memory profilers in field trials. Please go through the code. I have added my
comments, where I think code might have some issue.

#include <iostream>
#include <stdlib.h>

using namespace std;

typedef uint64_t BYTE8;
typedef unsigned char BYTE;

#define _AAANEW(ptr,structClass) \
try \
{ \
ptr = NULL; // Comment - not needed but see the catch block
ptr = new structClass; \
} \
catch(...) \
{ \
if(ptr == NULL) \
{ \
cout<<"Failure in _AAANEW, exiting the process";\
exit(0); \
} \
}

BYTE8 getTimerId()
{


BYTE* timer;
_AAANEW(timer,BYTE); //Comment - On 32 bit machine, timer will
//contain 32 bit virtual memory address
/* Timer id is taken as BYTE 8 since timer Id will be generated
on 32 bit an 64 bit also */
BYTE8 timerId = (BYTE8)timer; //Comment - I am not sure
// whether for 32 bit machine this
//conversion is safe

It is safe as long as BYTE8 is a large enough integer to hold the value of the
pointer (which should hold in your situation), but see below:

Theoretically, reinterpret_cast which is IMHO what is applied here for the
explicit cast conversion can change representation, so you might want to
double-check check you are not using some fancy union stuff when you are
converting the BYTE8 back to BYTE*.

return timerId;
}
thanks
Ittium

HTH
-Pavel

 

[naveen]

You can skip all the talk about virtual memory.

You are right. The process is running on linux 32 bit but new is
returning 64 bit address some time. I was wondering if virtual memory
addresses (return by new) for a process on 32 bit machine can be 64
bit.

C++ doesn't care
about that.  It simply guarantees that

  T* const a = new T();
  T* const b = new T();
  assert(a!=b);

for as long as *a and *b exist.





What a weird typedef!  What's wrong with uint64_t?











Ugh. I won't comment on that ...



OK, so yo allocate an ID by new:ing a small object, cast its address
to uint64_t, and that's the ID.  Presumably you never delete that
small object?

Since system starts large number of timers, we delete the object when
timer is stopped.

I see nothing wrong with it on systems with flat
addresses less than or equal 64-bits.  I think your bug is elsewhere.

By the way, why don't you just say "the timer ID is an unsigned
char*"?  Why bother to squeeze it into an integer?

Timers are kept in a std::map, where key is the timer id. We have
taken the timer id as
uint64_t so that code works for both 32 bit and 64 bit machines.

 

[naveen]

How do you determine when the timerId is junk?

We have taken gcore of running process, all of timer ids except few
are less than 32 bit. Few timer ids are more than 32 bit. Since
machine is 32 bit, I was wondering if new can return more than 32 bit
long address.

Are you trying to narrow that BYTE8 back to the pointer type and are not finding
the original BYTE8* there? How are you doing the reverse conversion?
















It is safe as long as BYTE8 is a large enough integer to hold the value of the
pointer (which should hold in your situation), but see below:

Theoretically, reinterpret_cast which is IMHO what is applied here for the
explicit cast conversion can change representation, so you might want to
double-check check you are not using some fancy union stuff when you are
converting the BYTE8 back to BYTE*.

While deleting the memory, we are just doing
delete((BYTE* )timerId); //Comment we should probably use reinterpret
cast.
timerId that is 8 byte. My question is, is this code unsafe on 32 bit
or 64 bit machine?. We have run it on both architecture without any
issue. We might be missing some use case where this code can fail or
corrupt the heap.

thanks Jorgen and Pavel for your time.

 

[Ian Collins]

You are right. The process is running on linux 32 bit but new is
returning 64 bit address some time. I was wondering if virtual memory
addresses (return by new) for a process on 32 bit machine can be 64
bit.

Highly unlikely considering sizeof(void*) will almost certainly be 4.

Since system starts large number of timers, we delete the object when
timer is stopped.


Timers are kept in a std::map, where key is the timer id.
We have taken the timer id as
uint64_t so that code works for both 32 bit and 64 bit machines.

Then use size_t if you want to do that. Or as suggested above, just use
a pointer type.

 

[Ian Collins]

We have taken gcore of running process, all of timer ids except few
are less than 32 bit. Few timer ids are more than 32 bit. Since
machine is 32 bit, I was wondering if new can return more than 32 bit
long address.

No.

If you just stick with a pointer type for your key, or use integer type
the same with as a pointer (size_t), you'll have a lot less to worry
about. Not only that, you can loose that awful typedef!

 

[Pavel]

We have taken gcore of running process, all of timer ids except few
are less than 32 bit. Few timer ids are more than 32 bit. Since
machine is 32 bit, I was wondering if new can return more than 32 bit
long address.

Oh, that.. my reading of the Standard is that there is no guarantee about the
representation of a 32-pointer cast to 64-bit unsigned. The only guarantee is
that the inverse conversion (from 64-bit unsigned to pointer) shall result in
the same value of the pointer (because 64-bit unsigned is "large enough" to hold
it).

A sensible implementation could, for example, leave the high 32-bit of 64-bit
unsigned uninitialized as soon as it is only using the low 32-bit during the
reverse conversions. It would spare it a CPU cycle or two on a 32-bit system.

You might want to try to change this your code:

BYTE8 timerId = (BYTE8)timer;

to

BYTE8 timerId = 0ULL;
timerId = (BYTE8)timer;

and check if those "greater than 32-bit" ids will keep popping up.

While deleting the memory, we are just doing
delete((BYTE* )timerId); //Comment we should probably use reinterpret
cast.
timerId that is 8 byte. My question is, is this code unsafe on 32 bit
or 64 bit machine?. We have run it on both architecture without any
issue. We might be missing some use case where this code can fail or
corrupt the heap.

thanks Jorgen and Pavel for your time.

HTH
-Pavel

 

[itt ium]

Oh, that.. my reading of the Standard is that there is no guarantee aboutthe
representation of a 32-pointer cast to 64-bit unsigned. The only guarantee is
that the inverse conversion (from 64-bit unsigned to pointer) shall result in
the same value of the pointer (because 64-bit unsigned is "large enough" to hold
it).

A sensible implementation could, for example, leave the high 32-bit of 64-bit
unsigned uninitialized as soon as it is only using the low 32-bit during the
reverse conversions. It would spare it a CPU cycle or two on a 32-bit system.

You might want to try to change this your code:

BYTE8 timerId = (BYTE8)timer;

to

BYTE8 timerId = 0ULL;
timerId = (BYTE8)timer;

and check if those "greater than 32-bit" ids will keep popping up.














HTH
-Pavel

Thanks Pavel, Pavo, Ian for your inputs.

I am going to make following changes

#1 change
timerId = (BYTE8)timer;
to
BYTE8 timerId = 0ULL;
timerId = (BYTE8)timer; // with static cast

#2 Add reinterpret cast in following
delete((BYTE* )timerId); // with reinterpret cast

I will post the results.

thanks
Ittium

 

[Rajat Gujral]

Thanks Pavel, Pavo, Ian for your inputs.

I am going to make following changes

#1 change
timerId = (BYTE8)timer;
to
BYTE8 timerId = 0ULL;
timerId = (BYTE8)timer; // with static cast

I dont think static cast is going to work here at all. Because as per code it seems you are trying to convert BYTE* to BYTE8. However reinterpret_castwould work here but i still suspect if its going to solve your problem. The best way to implement this is take a BYTE8 counter (as Paavo suggested).

In case you want to still pursue this approach then i would suggest to use bit operation to copy the value to BYTE8 which is safe operation based on the (size_t).

#2 Add reinterpret cast in following
delete((BYTE* )timerId); // with reinterpret cast

Since you are working on 32-bit i don't see any issue in converting back toBYTE* memory address. As system uses 4 byte of BYTE8 it surely will give memory address allocated earlier and rest of the 4 bytes are simple ignored.

I will post the results.

thanks
Ittium

cheers
Rajat