Pages: [1]
  Print  
Author Topic: Bug in favorites menu  (Read 5245 times)
sago007
Posts a lot
*

Cakes 62
Posts: 1664


Open Arena Developer


WWW
« on: June 19, 2007, 03:59:07 pm »

It is some time ago it happened, but by deleting a saved server address I somehow corrupted my favorites list.

While browsing through Code3Arena, I noticed this article: http://code3arena.planetquake.gamespy.com/tutorials/tutorial9.shtml
That tell about a bug in line 580 of "q3_ui/ui_server2.c"

It suggest that
Code:
memcpy( &g_arenaservers.favoriteaddresses[i], &g_arenaservers.favoriteaddresses[i+1], (g_arenaservers.numfavoriteaddresses - i - 1)*sizeof(MAX_ADDRESSLENGTH));
should be replaced by
Code:
memcpy( &g_arenaservers.favoriteaddresses[i], &g_arenaservers.favoriteaddresses[i+1], (g_arenaservers.numfavoriteaddresses - i - 1)*MAX_ADDRESSLENGTH);

that also makes more sense... MAX_ADDRESSLENGTH is a #define not a type. And its used without sizeof everywhere else in the file.
Logged

There are nothing offending in my posts.
beast
Lesser Nub


Cakes 0
Posts: 142



« Reply #1 on: August 02, 2007, 12:37:05 am »

The problem is definitely still in the current code and should be fixed. For what it's worth (I'm new here, so not worth much...), I agree that the recommended fix is the right one.
Logged
Kojiro_S
Lesser Nub


Cakes 2
Posts: 143


Har har har...


WWW
« Reply #2 on: August 02, 2007, 12:59:59 am »

So that's why that deleted server keeps coming back haha Tongue
Logged


*ROOOAAAR*
beast
Lesser Nub


Cakes 0
Posts: 142



« Reply #3 on: August 02, 2007, 02:11:44 am »

I looked through the code and here is the section that has the problem. It is q3_ui/ui_servers2.c starting at line 570...

Code:
// find address in master list
for (i=0; i<g_arenaservers.numfavoriteaddresses; i++)
if (!Q_stricmp(g_arenaservers.favoriteaddresses[i],servernodeptr->adrstr))
break;

// delete address from master list
if (i <= g_arenaservers.numfavoriteaddresses-1)
{
if (i < g_arenaservers.numfavoriteaddresses-1)
{
// shift items up
memcpy( &g_arenaservers.favoriteaddresses[i], &g_arenaservers.favoriteaddresses[i+1], (g_arenaservers.numfavoriteaddresses - i - 1)*sizeof(MAX_ADDRESSLENGTH));
}
g_arenaservers.numfavoriteaddresses--;
}

// find address in server list
for (i=0; i<g_numfavoriteservers; i++)
if (&g_favoriteserverlist[i] == servernodeptr)
break;

// delete address from server list
if (i <= g_numfavoriteservers-1)
{
if (i < g_numfavoriteservers-1)
{
// shift items up
memcpy( &g_favoriteserverlist[i], &g_favoriteserverlist[i+1], (g_numfavoriteservers - i - 1)*sizeof(servernode_t));
}
g_numfavoriteservers--;
}

Here's what I would propose to replace it with:
Code:
// find address in master list
for (i=0; i<g_arenaservers.numfavoriteaddresses; i++)
{
if (!Q_stricmp(g_arenaservers.favoriteaddresses[i],servernodeptr->adrstr))
{
// delete address from master list
if (i < g_arenaservers.numfavoriteaddresses-1)
{
// shift items up
memcpy( &g_arenaservers.favoriteaddresses[i], &g_arenaservers.favoriteaddresses[i+1], (g_arenaservers.numfavoriteaddresses - i - 1)*MAX_ADDRESSLENGTH);
}
g_arenaservers.numfavoriteaddresses--;
memset( &g_arenaservers.favoriteaddresses[g_arenaservers.numfavoriteaddresses], 0, MAX_ADDRESSLENGTH);
break;
}
}

// find address in server list
for (i=0; i<g_numfavoriteservers; i++)
{
if (&g_favoriteserverlist[i] == servernodeptr)
{
if (i < g_numfavoriteservers-1)
{
// shift items up
memcpy( &g_favoriteserverlist[i], &g_favoriteserverlist[i+1], (g_numfavoriteservers - i - 1)*sizeof(servernode_t));
}
g_numfavoriteservers--;
memset( &g_favoriteserverlist[g_numfavoriteservers], 0, sizeof(servernode_t));
break;
}
}

I cleaned up the code and made the change so that the correct length of bytes were being copied. I also added a memset to clear the last server that was in the list when everything got moved up. This is not totally necessary, but by clearing the data, we guarantee that an errant server will not show up because of some other problem in the code.

Obviously, the only 'necessary' change is the problem on line 580, so this whole message can be ignored if so desired... :-)
« Last Edit: August 02, 2007, 02:41:07 am by theBeast » Logged
dmn_clown
Posts a lot
*

Cakes 1
Posts: 1324


« Reply #4 on: August 03, 2007, 08:49:47 am »

Please do not post bugs and/or bug fixes that are not OA specific here, they will not get fixed.  Open a bugzilla account with upstream and file a bug in the ioq3 bug tracker.
Logged

beast
Lesser Nub


Cakes 0
Posts: 142



« Reply #5 on: August 03, 2007, 01:21:02 pm »

Thanks for the info. I wasn't sure how you guys handled this kind of issue. I will do as instructed. Sorry for an trouble.
Logged
dmn_clown
Posts a lot
*

Cakes 1
Posts: 1324


« Reply #6 on: August 03, 2007, 01:43:45 pm »

There is no trouble.

Just remember that only OA specific bugs belong on this board.
Logged

beast
Lesser Nub


Cakes 0
Posts: 142



« Reply #7 on: August 23, 2007, 03:22:42 pm »

I posted the fix upstream and it has been accepted and is in the code now. So, OA should see the fix next time the ioQuake code is updated.
Logged
Pages: [1]
  Print  
 
Jump to: