Bug in favorites menu

[sago007]

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.

[beast]

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.

[Kojiro_S]

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

[beast]

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

[dmn_clown]

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.

[beast]

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.

[dmn_clown]

There is no trouble.

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

[beast]

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.