Ticket #5887 (closed patch: fixed)

Opened 21 months ago

Last modified 12 months ago

Pidgin does not work on 64-bit userland PowerPC systems

Reported by: JoseJX Owned by:
Milestone: 2.5.5 Component: libpurple
Version: 2.4.2 Keywords: ppc64
Cc:

Description

The problem:

On 64-bit systems, time_t is a 64-bit value. time_t is used as a hash key in savedstatuses.c, but the g_hash_table is implemented using a 32-bit hash key value. Additionally, all of the preferences save these keys as 32-bit values. Unfortunately, the first word is used on 64-bit architectures. On little endian systems, these are the LSB and no problem is visible. On big endian 64-bit systems, the values used for the hash key are the MSB and do not change, leading to collisions, etc.

This manifests as looping for a *very* long time when adding new hash elements, as well as segfaults due to an inability to match keys.

Proposed solution:

Change the current usage of time_t in saved_statuses to unsigned int. This fixes the observed issue on ppc64.

Attachments

savedstatuses_ppc64.patch (3.8 kB) - added by JoseJX 21 months ago.
Changes time_t usage to int so that the hash table works properly on BE 64-bit systems

Change History

Changed 21 months ago by JoseJX

Changes time_t usage to int so that the hash table works properly on BE 64-bit systems

Changed 21 months ago by rlaager

  • milestone set to Patches Needing Improvement

This patch changes the signatures of public methods (unless I read it wrong). I haven't looked into this issue, but from the description, I wonder: Could we just convert them before handing them to the hash table functions?

Changed 21 months ago by JoseJX

Well, that was my initial thought, but you're only keeping 32-bits of the value when you save/read preferences. Also, there's a number of comparisons between int and the time values, so I thought it would be better to simply use the same type throughout.

Changed 21 months ago by rlaager

Changing APIs requires a major version bump. What about adding new API to allow saving time_t values or 64-bit integer values?

Changed 21 months ago by JoseJX

I've looked into that a bit. Basically, you'll need to make g_hash do time_t hashing, since time_t can be 64 bit or 32 bit, but shouldn't be too hard as long as the pointer size is the same as time_t (you can use g_direct_hash). The more difficult part is adding preferences loading and storage and ensuring that all uses compare with time_t instead of the int type.

It seemed much simpler to just modify the interface as I've done here, I don't think I have the time to figure out how the preferences system works, sorry.

Changed 12 months ago by qulogic@…

  • status changed from new to closed
  • resolution set to fixed
  • milestone changed from Patches Needing Improvement to 2.5.5

(In [0ed824d0e7a548cb9a2fc4da692270475567d7d6]):
Use g_direct_hash/equal instead of g_int_hash/equal for the hash table that holds the saved statuses. The effect is that this will take into account the different size of time_t across 32-bit and 64-bit systems. There should not be any side effects because it is only used at runtime for the hash key, and all the important data is stored in the value.

This should stop any mysterious infinite loop problems on big-endian systems which were trying to use the MSB of the time as a hash key.

Fixes #5887.

Note: See TracTickets for help on using tickets.