Issues in generating unique time id using virtual memory address

Discussion in 'C++' started by ittium, Jun 10, 2012.

  1. ittium

    ittium Guest

    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
     
    ittium, Jun 10, 2012
    #1
    1. Advertising

  2. ittium

    Jorgen Grahn Guest

    On Sun, 2012-06-10, ittium wrote:
    > 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

    --
    // Jorgen Grahn <grahn@ Oo o. . .
    \X/ snipabacken.se> O o .
     
    Jorgen Grahn, Jun 10, 2012
    #2
    1. Advertising

  3. ittium

    Pavel Guest

    ittium wrote:
    > 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
     
    Pavel, Jun 11, 2012
    #3
  4. ittium

    naveen Guest

    On Jun 11, 1:19 am, Jorgen Grahn <> wrote:
    > On Sun, 2012-06-10, ittium wrote:
    > > 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.


    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.
    >
    > > 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?


    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.

    >
    > /Jorgen
    >
    > --
    >   // Jorgen Grahn <grahn@  Oo  o.   .     .
    > \X/     snipabacken.se>   O  o   .
     
    naveen, Jun 11, 2012
    #4
  5. ittium

    naveen Guest

    On Jun 11, 4:56 am, Pavel
    <> wrote:
    > ittium wrote:
    > > 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?


    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?
    >
    >
    >
    >
    >
    >
    >
    >
    >
    > > I doubt a possible heap corruption, but we can not run
    > > memory profilers in field trials.  Please go through the code. I haveadded 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*.


    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.



    >
    > >       return timerId;
    > > }
    > > thanks
    > > Ittium

    >
    > HTH
    > -Pavel
     
    naveen, Jun 11, 2012
    #5
  6. ittium

    Ian Collins Guest

    On 06/11/12 05:02 PM, naveen wrote:
    > On Jun 11, 1:19 am, Jorgen Grahn<> wrote:
    >> On Sun, 2012-06-10, ittium wrote:
    >>> 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.

    >
    > 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.

    >> 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.


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

    --
    Ian Collins
     
    Ian Collins, Jun 11, 2012
    #6
  7. ittium

    Ian Collins Guest

    On 06/11/12 05:10 PM, naveen wrote:
    > On Jun 11, 4:56 am, Pavel
    > <> wrote:
    >> ittium wrote:
    >>> 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?

    >
    > 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!

    --
    Ian Collins
     
    Ian Collins, Jun 11, 2012
    #7
  8. ittium

    Pavel Guest

    naveen wrote:
    > On Jun 11, 4:56 am, Pavel
    > <> wrote:
    >> ittium wrote:
    >>> 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?

    >
    > 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.


    >


    >>
    >> 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*.

    >
    > 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.
    >
    >
    >
    >>
    >>> return timerId;
    >>> }
    >>> thanks
    >>> Ittium

    >>
    >> HTH
    >> -Pavel

    >



    HTH
    -Pavel
     
    Pavel, Jun 12, 2012
    #8
  9. ittium

    itt ium Guest

    On Jun 12, 8:37 am, Pavel
    <> wrote:
    > naveen wrote:
    > > On Jun 11, 4:56 am, Pavel
    > > <> wrote:
    > >> ittium wrote:
    > >>> 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?

    >
    > > 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 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.
    >
    >
    >
    >
    >
    >
    >
    >
    >
    >
    >
    > >> 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 forthe
    > >> 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.

    >
    > >>>        return timerId;
    > >>> }
    > >>> thanks
    > >>> Ittium

    >
    > >> HTH
    > >> -Pavel

    >
    > 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
     
    itt ium, Jun 12, 2012
    #9
  10. ittium

    Rajat Gujral Guest

    On Tuesday, 12 June 2012 16:10:32 UTC+5:30, itt ium wrote:
    > On Jun 12, 8:37 am, Pavel
    >
    > wrote:
    > > naveen wrote:
    > > > On Jun 11, 4:56 am, Pavel
    > > >

    > wrote:
    > > >> ittium wrote:
    > > >>> Group,
    > > >>> I came across a timer implementation where to generate unique timerid,
    > > >>> implementer has used virtual memory  address. In the program written below,
    > > >>> using new, 1 byte memory is allocated, this memory is kept reservedtill 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?

    > >
    > > > 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.
    > >
    > >
    > >
    > >
    > >
    > >
    > >
    > >
    > >
    > >
    > >
    > > >> Are you trying to narrow that BYTE8 back to the pointer type and arenot 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 catchblock
    > > >>>       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 notsure
    > > >>>                   // 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 youare
    > > >> 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.

    > >
    > > >>>        return timerId;
    > > >>> }
    > > >>> thanks
    > > >>> Ittium

    > >
    > > >> HTH
    > > >> -Pavel

    > >
    > > 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


    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
     
    Rajat Gujral, Jun 14, 2012
    #10
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Curt_C [MVP]

    Re: Generating 8 digit unique ID

    Curt_C [MVP], Apr 20, 2004, in forum: ASP .Net
    Replies:
    1
    Views:
    490
    Curt_C [MVP]
    Apr 20, 2004
  2. Krishna Sagiraju

    convert virtual address to physical address

    Krishna Sagiraju, Jul 10, 2004, in forum: C Programming
    Replies:
    3
    Views:
    1,267
    Malcolm
    Jul 10, 2004
  3. sushant

    virtual address or physical address?

    sushant, Feb 18, 2005, in forum: C Programming
    Replies:
    9
    Views:
    1,039
    CBFalconer
    Feb 18, 2005
  4. ToshiBoy
    Replies:
    6
    Views:
    888
    ToshiBoy
    Aug 12, 2008
  5. Token Type
    Replies:
    9
    Views:
    393
    Chris Angelico
    Sep 9, 2012
Loading...

Share This Page