Opened 8 years ago

Closed 8 years ago

#13370 closed patch (fixed)

libpurple jabber does not process avatar removal updates

Reported by: mentor Owned by: deryni
Milestone: 2.8.0 Component: XMPP
Version: 2.7.10 Keywords:
Cc:

Description

libpurple does not process the case where the avatar hash is not present and does not ever set an empty avatar.

This patch extends difference checking to empty avatars, and also sets empty avatar data.

Attachments (3)

libpuple-jabber-no-avatar.diff (2.2 KB) - added by mentor 8 years ago.
libpurple-jabber-no-avatar.diff (2.6 KB) - added by mentor 8 years ago.
Version 2
libpurple-jabber-no-avatar.2.diff (2.9 KB) - added by mentor 8 years ago.

Download all attachments as: .zip

Change History (19)

Changed 8 years ago by mentor

comment:1 Changed 8 years ago by Robby

  • Milestone set to Patches Needing Review

Thanks a lot! :)

comment:2 Changed 8 years ago by QuLogic

  • Component changed from libpurple to XMPP
  • Owner set to deryni

comment:3 Changed 8 years ago by darkrain42

  • Milestone changed from Patches Needing Review to Patches Needing Improvement
  • Status changed from new to pending

The logic for determining to unset an avatar appears incorrect to me. It should check for an empty <photo/> element (see XEP-0153 4.1 #3):

If there is no avatar image to be advertised, the photo element MUST be empty, i.e.:
...
Note: This enables recipients to distinguish between the absence of an image (empty photo element) and mere support for the protocol (empty update child)

I also vehemently object to this code on readability grounds: if (!!ah != !!ah2...

comment:4 Changed 8 years ago by darkrain42

  • Status changed from pending to new

Oops

comment:5 follow-up: Changed 8 years ago by mentor

To the best of my knowledge and belief, !!a != !! is the canonical form of a logical XOR in C.

comment:6 in reply to: ↑ 5 Changed 8 years ago by mentor

Replying to mentor:

!!a != !!

Arg. !!a != !!b

comment:7 Changed 8 years ago by mentor

presence->vcard_avatar_hash contains NULL for an empty photo or the photo hash. presence->vcard_avatar_hash is updated from parse_vcard_avatar(), which only does so if there is a photo element present. As such, I believe the logic is correct.

Changed 8 years ago by mentor

Version 2

comment:8 Changed 8 years ago by mentor

Ah! Of course, presence->vcard_avatar_hash is not initialised to the old avatar hash. An updated version of the patch has vcard_avatar_hash as NULL for no update, "" for empty, and strong values as hash.

Please feel free to update the XOR code to something you find more readable.

comment:9 Changed 8 years ago by darkrain42

  • Milestone changed from Patches Needing Improvement to Patches Needing Review

comment:10 Changed 8 years ago by darkrain42

  • Milestone changed from Patches Needing Review to Patches Needing Improvement

A few observations (mostly from deryni):

  • The logic in the first chunk is still wrong (see my previous note)
  • const char *ah = presence->vcard_avatar_hash != "" ? <-- not equals "" is very likely wrong
  • Can't the XOR be replaced with a purple_strequal(string1, string2)? (all you're looking for is whether the strings are equal or not, right?)
  • ?: is a GNUism.

Also, your indentation is off (there are extraneous indentation changes).

Changed 8 years ago by mentor

comment:11 Changed 8 years ago by mentor

  • Milestone changed from Patches Needing Improvement to Patches Needing Review

purple_strequal does have the correct semantics, yes.

comment:12 Changed 8 years ago by darkrain42

mentor, what name and email address should be used for credit/COPYRIGHT?

comment:13 Changed 8 years ago by mentor

Matthew W.S. Bell <matthew@…>

comment:14 Changed 8 years ago by Robby

  • Milestone changed from Patches Needing Review to 2.8.0

comment:15 Changed 8 years ago by Robby

  • Type changed from defect to patch

comment:16 Changed 8 years ago by matthew@…

  • Resolution set to fixed
  • Status changed from new to closed

(In f02baf428501d0fb764628538fa150c7111e789f):
jabber: Correctly handle the unsetting case for a contact's vCard avatar.

Patch from Matthew (mentor) W.S. Bell. Closes #13370

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!