Bugs/Buglets I've found and their fixes

[Osiri]

Ok folks.. here goes: (current as of my checkout from CVS about 3 hours prior to this posting)

PS: any of these may be wrong.. submitted for approval only

Issue:
Null Pointer Read:
Where:
    psCelClient::HandleUnresolvedPos(GEMClientObject *,csVector3 const&,float,csString const&) [g:\\dev\\planeshift\\planeshift\\src\\client\\pscelclient.cpp:751]
Fix:
          /* TODO: begin change */
            if (psengine->GetCharControl())
         {
               psengine->GetCharControl()->GetMovementManager()->StopAllMovement();
            }
           /* old version */
           //psengine->GetCharControl()->GetMovementManager()->StopAllMovement();
          /* end change */

Issue:
Nit pick (hard to read, possible uninited memory read)
Where:
psMsgStringsMessage::psMsgStringsMessage(MsgEntry *message) [messages.cpp:3054]
Fix:
       /* TODO: begin change */
        char * tpos = buff + pos;
       uint32 *p = (uint32*)tpos;
      /* old version */
       //uint32 *p = (uint32*)(buff+pos);  //change for readability
        /* end change */

Issue:
Unitialized Memory Read
Where:
void pawsMultiLineTextBox::Draw() [pawstextbox.cpp:1511]
Fix:
Ok this is a two part fix.. First of all canDrawLines should be inited in the constructor to 0.
The second part is as follows at the offending location.  However, I\'m not 100% sure this is the right fix.  It\'s just the best I could find.
[edit] Ok this still didn\'t solve the unitied memory read, although it REALLY should have.  canDrawLines is STILL united.  Is there a problem with: CREATE_PAWS_FACTORY??? [/edit]

  /* TODO: begin change */
    // This is possibly the wrong course of action but canDrawLines wasn\'t inited in constructor.. best I could come up with
 if (!canDrawLines) canDrawLines = screenFrame.Height() / maxHeight;
    for (size_t x = startLine; x < (startLine+canDrawLines); x++ )
  /* old version */
   //for (size_t x = startLine; x < (startLine+canDrawLines); x++ )
    /* end change */

Regards,

Osiri

[LigH]

/me votes for Osiri into DEV team!

I already witnessed your ideas in IRC, you know what you\'re talking about.

[Nyramael]

006.03.01

     
We have been needing to do something to improve our recruitment process for a long time. The interest of the community is everyday bigger and many people want to apply for development. This requires a lot of time on our side to manage all the prospects, assign tasks, check progress and track the status of old ones. We decided to put an end to this by creating an application that handles all the prospect applications and tasks.

The recruit application is now linked from our recruit page.

Please remember that we are searching only talented and committed people that really want to help PlaneShift.

-from the main PS website.

/me seconds that notion

Osiri i think we\'d have a lot to gain if you joined the dev team. It does seem like you know what you\'re talking about.

[Wired_Crawler]

Originally posted by Osiri
Fix:
         /* TODO: begin change */
            if (psengine->GetCharControl())
         {
               psengine->GetCharControl()->GetMovementManager()->StopAllMovement();
            }
           /* old version */
           //psengine->GetCharControl()->GetMovementManager()->StopAllMovement();
          /* end change */

I\'m just curious - could anybody explain, why shouldn\'t it be like the following ?

Code: [Select]


          /* TODO: begin change */
            if (psengine->GetCharControl())
         {
           if (psengine->GetCharControl()->GetMovementManager())
               {
               if (psengine->GetCharControl()->GetMovementManager()->StopAllMovement())
                        {
                       psengine->GetCharControl()->GetMovementManager()->StopAllMovement();
                        }
                   }
               }


[AryHann]

eheh ;-)

Well, I think that the problem is that we get a null element sometimes when charcontrol is not available and you want to turn off the movement.

The issue is that adding the check for null might not be the complete solution for the problem, because theoretically that function shouldn\'t be called if not available.. I mean, it could be a the order of calling the functions that could be wrong. So the problem is to be investigated and it is not said that that is the solution.

[Nilrem]

Originally posted by Wired_Crawler
I\'m just curious - could anybody explain, why shouldn\'t it be like the following ?

Mmmm.
We\'ll perhaps I\'d sound a bit nerd, since I haven\'t read the code, but hey, I\'ll try to answer you

The succesive comprovations that you added, to see if they returned true before digging more inside, could be due to this:

Whatever GetCharControl returns seems to be a class that contains the function GetMovementManager that returns bla bla...

Is your solution wrong/the right one?
I don\'t know, depends on the coding.

Don\'t get me wrong, yours seems \"gramatically correct\" but it may have unnecesary checks.

For instance:
Perhaps, earlier in the code, whenever a returned_by_GetCharControl class is is created, it already correctly initializes all its parameters, so your further checks are innecessary, since, as for the code is built, those already are correctly initialized, pointing to somewhere valid.
It maybe not, so then further checkings would be necessary, but to discover that I guess you\'d really dig inside the code, and I\'m for sure the wrong one to do that So I hope my answer made some sense to you.

PS: And yeah, all pointers should be initialized to some valid memory position, and yeah, there\'s a malloc thingy to be used somewhere and yeah... but hey, what do I know? I\'m only answering this as a matter of politeness

[acraig]

As a bit of an aside, checking all NULL pointers like this might not be desirable.  If there is a condition where something should never be NULL and we do these checks there is the possiblity that something is silently failing.  If the system hard crashes in a spot that should be impossible to get a NULL pointer we know that there is a problem elsewhere instead of just doing a silent failure where we are not really aware of an impossible case happening.

The \'right\' way to do this would be with an exception and log to file and do a clean shut down.

[Osiri]

acraig and Ary:

I agree this probably isn\'t the ultimate fix for the problem.  The problem happened for my on my client at load into the world (bad position).

I will however say that, I don\'t need to stopmovement if I\'m not actually moving, so while it\'s not perfect, and probably should be logged (agree 100%) it\'s also a non-fatal issue.  

Should it be resolved elsewhere: quite possibly.  Does it break anything resolving it here: not from my testing.

Osiri

[DaveG]

Originally posted by Osiri
Issue:
Nit pick (hard to read, possible uninited memory read)
Where:
psMsgStringsMessage::psMsgStringsMessage(MsgEntry *message) [messages.cpp:3054]
Fix:
        /* TODO: begin change */
        char * tpos = buff + pos;
        uint32 *p = (uint32*)tpos;
        /* old version */
        //uint32 *p = (uint32*)(buff+pos);  //change for readability
        /* end change */

Interesting...  What brought your eye to this?  Did a warning popup, or were you just browsing through the code?

I\'m reading an ID from a buffer here, which I had just filled up from decompressing the payload of an incoming psMsgStringsMessage.  It admittedly isn\'t the easiest to read if you don\'t understand some more of the networking code (take a look at message.h), and we try to limit pointer arithmetic most of the time.    However, the way it\'s written is the cleanest you\'re going to get for grabbing something at that location.  Think:

(type of pointer I want to cast to)(starting point + distance traveled = location I want)

Jumping temporarily to a char* just complicates things further, in my view.  That would turn a 3 step process into 4:

Code: [Select]

[i]Original:[/i]
        // Unpack ID
        uint32 *p = (uint32*)(buff+pos);
        uint32 id = csLittleEndian::UInt32(*p);
        pos += sizeof(uint32);

[i]Your version:[/i]
        // Unpack ID
        char * tpos = buff + pos;
        uint32 *p = (uint32*)tpos;
        uint32 id = csLittleEndian::UInt32(*p);
        pos += sizeof(uint32);
If I really wanted to smush it down, I could cram all of that into one line, but that\'d obviously be rather ugly.  It just comes down to how many steps you do at once.  Frankly, if the standard way to do this operation in the networking code was the latter, I\'d probably maintain that for consistency.  But, there\'s so much pointer math around here, that it\'s pretty much all written in the simplest form, which isn\'t necisarilly the simplest to read if you aren\'t familiar with it.  But, as you already said, this is nit-picking.  

As you see below that code, I break the loop on \"pos > z.total_out\" which would indicate I\'m about to read past the end.  If this were to be reading past the end of the buffer, that would mean something went horribly wrong, and I would expect that to crash in its own right.

When it comes to the first one of your changes (Null Pointer Read), the core problem there is that it\'s probably processing a message long before it is capable of doing so.  Instead of a NULL check there, we\'ll need to trace back to why the message is used at all.  The 3rd one has been crashing for a while, and if you can find a good fix for that, please do.  

[Osiri]

Dave:

What brought it to my eye is that for some reason it got flagged as a UMR (uninitalized memory read).  I was trying to simplify it so I could understand it a bit more.  Having said that I agree that adds confusion and would actually upon reflection (personally) move the * around a bit:

uint32 p = *(uint32*)(buff+pos);
uint32 id = csLittleEndian::UInt32(p);

I think that\'s the best of both worlds.. both easily readable and short.

As for the 3rd item.  I have to look at the class factory code.. something is off.

Osiri

[DaveG]

Yep.  I did that whole block of code manually, so I could compress the whole thing more easily.  I should probably break it down into smaller functions, as I might want to compress a few other large messages as well.