Potential bug in EvtData_New_Actor and EvtData_Request_New_Actor

    This site uses cookies. By continuing to browse this site, you are agreeing to our Cookie Policy.

    • Potential bug in EvtData_New_Actor and EvtData_Request_New_Actor

      Found something in the Events.h file:

      Source Code

      1. virtual ~EvtData_New_Actor()
      2. {
      3. SAFE_DELETE(m_pActorParams);
      4. }


      "SAFE_DELETE" is a macro for "delete ...", but the memory block is allocated with new[] in the contructor:

      Source Code

      1. explicit EvtData_New_Actor( ActorId id, ActorParams *pCreateParams)
      2. {
      3. m_id = id;
      4. m_pActorParams = reinterpret_cast<ActorParams *>(GCC_NEW char[pCreateParams->m_Size]);
      5. memcpy(m_pActorParams, pCreateParams, pCreateParams->m_Size);
      6. m_pActorParams->m_Id = id;
      7. }


      The right thing to do in my eyes is to use delete[]. Unfortunately this leads to crashes... but this can be easily fixed. Here is the new code for the destructor:

      Source Code

      1. virtual ~EvtData_New_Actor()
      2. {
      3. if (m_pActorParams)
      4. {
      5. delete[] reinterpret_cast<char*>(m_pActorParams);
      6. }
      7. }
      9. ^^ faulty code, check my next post why


      The exact same thing applies to the EvtData_Request_New_Actor destructor.

      The post was edited 1 time, last by Nemex ().

    • I'm working on a GUI system at the moment and hooked it up to the script side today. In the process I noticed that my fix for this problem isn't perfect at all. The basic problem was that there is a new[]/delete mismatch. My fix did some additional casts to do a matching delete[] call. But today I noticed that actor params are allocated at other points with new:

      Source Code

      1. Events.cpp
      3. Line 70: ActorParams *ActorParams::CreateFromStream(std::istrstream &in)
      5. Line 116: ActorParams * ActorParams::CreateFromLuaObj( LuaObject srcData )


      I did some quick thinking and came to an even more elegant solution in my opinion. I already did some prework for it to work, so I'll go through it at first:

      I added another static method to the ActorParams class:

      Source Code

      1. Actors.h, Line 132:
      3. static ActorParams * Allocate(ActorType type);


      Implementation:

      Source Code

      1. Events.cpp, at end:
      3. ActorParams * ActorParams::Allocate(ActorType type)
      4. {
      5. ActorParams actor = NULL;
      7. switch (type)
      8. {
      9. case AT_Sphere:
      10. actor = GCC_NEW SphereParams;
      11. break;
      13. case AT_Teapot:
      14. actor = GCC_NEW TeapotParams;
      15. break;
      17. /* more types go here */
      19. default:
      20. assert(0 && _T("Unimplemented actor type in stream"));
      21. return NULL;
      22. }
      24. return actor;
      25. }
      Display All


      This is now called from ActorParams::CreateFromStream and ActorParams::CreateFromLuaObj methods. This puts the allocation with "new" into exactly one place which is important for the next step. The EvtData_New_Actor constructor is changed in the following way:

      Source Code

      1. explicit EvtData_New_Actor(ActorId id, ActorParams *pCreateParams)
      2. {
      3. m_id = id;
      5. /* this is gone
      6. m_pActorParams = reinterpret_cast<ActorParams *>(GCC_NEW char[pCreateParams->m_Size]);
      7. and replaced with the following */
      9. m_pActorParams = ActorParams::Allocate(pCreateParams->m_Type);
      11. memcpy(m_pActorParams, pCreateParams, pCreateParams->m_Size);
      12. m_pActorParams->m_Id = id;
      13. }
      Display All


      The same thing must be done in the EvtData_Request_New_Actor constructor.

      I hope I got it right this time. Please let me know any thoughts or errors I made. Furthermore I may note that I have a heavily modified version of this code running, so I may have missed something easily.

      The post was edited 2 times, last by Nemex ().