Opened 6 years ago

Closed 4 years ago

Last modified 2 years ago

#12637 closed defect (fixed)

Pdgin crash when accepting a sametime chatroom invitation

Reported by: angejudor Owned by: siege
Milestone: 2.10.7 Component: Sametime
Version: 2.8.0 Keywords: sametime chatroom crash windows
Cc:

Description (last modified by angejudor)

Hello,

I have a crash of the pidgin application when I accept a chatroom invitation (sametime protocol). It's occuring since our company upgraded the sametime server to the 8.5.1 level.

I'm using 2.8.0 pidgin release on windows.

pidgin.2.RPT attached to this message (generated with the 2.8.0 pidgin release).

Thanks for the help,

Ange.

PS : on the debug info file I have the following lines :

(14:15:05) meanwhile: place PLC$51027737236915A8 state: pending
(14:15:05) meanwhile: channel accepted with encrypt policy 0x2000
(14:15:05) meanwhile: channel 0x00000006 selected cipher RC2/128 Cipher
(14:15:05) meanwhile: channel 0x00000006 state: open
(14:15:05) meanwhile: place PLC$51027737236915A8 state: joining
(14:15:05) meanwhile: place PLC$51027737236915A8 state: joined
(14:15:05) meanwhile: mwString_get: assertion `check_buffer(b, (gsize) len)' failed
(14:15:05) meanwhile: place PLC$51027737236915A8 state: open
(14:15:05) meanwhile: collected member 2: CN=Laurent XX/OU=YYY/O=ZZZ, (null)
(14:15:05) meanwhile: collected member 1028415861: (null), (null)
(14:15:05) sametime: place PLC$51027737236915A8 opened, 2 initial members
(14:15:05) g_log: purple_normalize: assertion `str != NULL' failed
(14:15:05) g_log: purple_find_buddy: assertion `(name != NULL) && (*name != '\0')' failed
(14:15:05) g_log: purple_conv_chat_is_user_ignored: assertion `user != NULL' failed
(14:15:05) g_log: purple_conv_chat_cb_new: assertion `name != NULL' failed
(14:15:05) g_log: purple_find_buddy: assertion `(name != NULL) && (*name != '\0')' failed

Attachments (4)

pidgin.RPT (6.8 KB) - added by angejudor 6 years ago.
pidgin.2.RPT (4.0 KB) - added by angejudor 5 years ago.
Report of the crash with the 2.8.0 release
srvc_place.c (23.6 KB) - added by jricesterenator 4 years ago.
preliminary meanwhile library fix
grpinvite (244 bytes) - added by jricesterenator 4 years ago.
Dump of incoming group chat invite. View with a hex editor to match up the format. This invite is a little strange because I'm inviting myself to the group chat.

Download all attachments as: .zip

Change History (40)

Changed 6 years ago by angejudor

comment:1 follow-up: Changed 6 years ago by snorkel

Any chance of getting this fixed for the next release? Pidgen flat out does not work on 8.5.1 sametime servers unless a server side INI file setting is made and that's not a acceptable solution.

comment:2 in reply to: ↑ 1 Changed 6 years ago by ravikiran

We are using Pidgin in linux and we face same issue.

Can someone please fix this at the earliest? We use pidgin as our SameTime? client and now it does not work.

BTW, is this an issue Pidgin or meanwhile library ?

comment:3 Changed 5 years ago by angejudor

I'm still having the same crash with latest pidgin release 2.8, Is someone working on this issue ?

Thanks for the help,

Ange.

comment:4 Changed 5 years ago by angejudor

  • Version changed from 2.7.3 to 2.8.0

Changed 5 years ago by angejudor

Report of the crash with the 2.8.0 release

comment:5 Changed 5 years ago by angejudor

  • Description modified (diff)

comment:6 Changed 5 years ago by angejudor

  • Description modified (diff)

comment:7 Changed 5 years ago by thb

This problem still exists in 2.10.0

comment:8 Changed 4 years ago by jricesterenator

This issue has been killing me for a while since a sametime upgrade. It's for sure an issue for at least pidgin 2.5.5 through 2.10.1.

For me, it only happens when someone (via their official client) invites me to a group chat from an existing chat window using "invite others". If they invite me to an "Instant Meeting" from their buddy list, everything goes ok.

For anyone debugging, here are steps to reproduce it on your own:

  1. Load Pidgin
  2. Load official sametime client
  3. From the official client, open a chat with yourself and send a message.
  4. You should see output in the official client and in pidgin.
  5. From the official client, "invite others" and invite yourself. Send.
  6. Pidgin crashes.

Over the next while, I'm hoping to start looking at the code to fix this.

Last edited 2 years ago by jricesterenator (previous) (diff)

comment:9 Changed 4 years ago by jricesterenator

Whelp, the issue is in the Meanwhile library. The group invite now contains an extra name field, and the library can't handle it. I have a preliminary fix that I think works on Linux, now I just need to figure out how to compile this on Windows..

Last edited 2 years ago by jricesterenator (previous) (diff)

comment:10 Changed 4 years ago by jhkrischel

@ jricesterenator: Can you give some specifics on your preliminary fix (specific file in the meanwhile library)?

I'd love to solve this problem for Adium, and would appreciate any details you can share!

Changed 4 years ago by jricesterenator

preliminary meanwhile library fix

Changed 4 years ago by jricesterenator

Dump of incoming group chat invite. View with a hex editor to match up the format. This invite is a little strange because I'm inviting myself to the group chat.

comment:11 Changed 4 years ago by jricesterenator

I've attached the code for the preliminary meanwhile library fix I've been working on. The group chat invite contains an extra name field for each user that must be parsed out.

See my changes on lines 168, 398, 550.

I don't know how to determine if the message is in the old or new format, so you'd have to use the right library version for your situation. Can anyone help with making the fixed version conditional?

(still working on compiling for win32 so I can do a proper test for my situation)

comment:12 Changed 4 years ago by jhkrischel

Cool, thanks jricesterenator!

comment:13 Changed 4 years ago by jhkrischel

Not that I'm all too bright at C, but maybe a null check when we look for &m->extraname can make it flexible enough to handle all conditions?

For Adium, I've noticed that group chat invites *from* adium don't kill me, but group chat invites *from* the official client do. I'll try it both ways and see if this patch breaks the way that works right now.

comment:14 Changed 4 years ago by jhkrischel

I'm guessing this might be a workable conditional:

if(&m->extraname) {

mString_get(b, &m->extraname);

}

comment:15 Changed 4 years ago by jhkrischel

BTW, the fix works on Adium both ways (without a conditional) - with an official client invite, and with an adium invite, so GREAT WORK jricesterenator!!!!

comment:16 follow-up: Changed 4 years ago by jricesterenator

@jhkrischel Glad to hear it's working. To summarize from our conversation last night, these scenarios also need to be tested.

  1. What happens if the fixed code receives an old-style message when the Pidgin user is invited to a chat that already has other people in it? The chat invite message sends a list of all the users, so testing with one person might not show some related issues.
  2. Need to make sure that someone joining the chat works too.

I'm not a C pro either, but I'm not sure that conditional would work. As background, mString_get(...) grabs the next string element from the buffer, advances a position counter, and stores the result in m->extraname. So m->extraname won't really exist until you call mString_get(b, &m->extraname).

Other ways I was thinking of were: 1) Determine the how long a single user definition section is and compare that to and old-style length or a new-style length. What's tricky with this is that the message isn't fixed length. Each string section is 2 bytes telling the length of the string, followed by that many bytes of characters. 2) Before each user section, the code skips some bytes(mwGetBuffer_advance). Maybe there are control characters in there that mark the beginning of a record.

comment:17 Changed 4 years ago by jhkrischel

Doing the testing today, but in the meantime, someone on the adium side did mention that we probably need to add a free statement to the patch:

http://trac.adium.im/ticket/16114

comment:18 Changed 4 years ago by jricesterenator

Yeah, they're right about the free.

Also, can we say that testing against a group chat invite created by the meanwhile lib (via pidgin or adium) is the same as testing against an official pre-8.5.1 invite? They should be compatible, but it'd be nice to test against the real thing somehow..

comment:19 in reply to: ↑ 16 ; follow-up: Changed 4 years ago by jhkrischel

Replying to jricesterenator:

I'm not a C pro either, but I'm not sure that conditional would work. As background, mString_get(...) grabs the next string element from the buffer, advances a position counter, and stores the result in m->extraname. So m->extraname won't really exist until you call mString_get(b, &m->extraname).

Okay, so when the mwString_get(b, &m->extraname); call is made, there seem to be two options -> either b (the buffer with the data) will be valid and we'll set &m->extraname, or b will be null, and the mwString_get(...) function will hit this line:

g_return_if_fail(b != NULL);

The way I read that is that if b == NULL, it will return. mw_str_get() returns void, so my guess is that it gracefully fails.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 4 years ago by jricesterenator

Replying to jhkrischel:

Replying to jricesterenator:

I'm not a C pro either, but I'm not sure that conditional would work. As background, mString_get(...) grabs the next string element from the buffer, advances a position counter, and stores the result in m->extraname. So m->extraname won't really exist until you call mString_get(b, &m->extraname).

Okay, so when the mwString_get(b, &m->extraname); call is made, there seem to be two options -> either b (the buffer with the data) will be valid and we'll set &m->extraname, or b will be null, and the mwString_get(...) function will hit this line:

g_return_if_fail(b != NULL);

The way I read that is that if b == NULL, it will return. mw_str_get() returns void, so my guess is that it gracefully fails.


The problem I'm seeing is not if the data runs out, it's when reading a message when >1 user is listed (and even the simplest message has at least 2 users...you and the requestor).

Here's a simplified example:

Old-style message:

Header
user1-placeId
user1-loginId
user1-name
user2-placeId
user2-loginId
user2-name
user3-placeId
user3-loginId
user3-name

New-style message:

Header
user1-placeId
user1-loginId
user1-name
user1-extraname
user2-placeId
user2-loginId
user2-name
user2-extraname
user3-placeId
user3-loginId
user3-name
user3-extraname

Now imagine we're running the fixed code and receive an old-style message with multiple users listed in it. Sure, the graceful fail would come into play after reading user3-name. The problem is that after user1-name, there's still 2 more users worth of data. So when we try to parse out the missing user1-extraname, it won't fail, but instead it'll grab user2-placeId data. And from there on out everything gets offset and is reading the wrong data.

In the simple example, data is just stored in the wrong fields. In a real situation, the wrong bytes would be parsed and it'd end up a jumble of nothing.

Incoming old-style message Parsed as
HeaderHeader
user1-placeIduser1-placeId
user1-loginIduser1-loginId
user1-nameuser1-name
user2-placeIduser1-extraname
user2-loginIduser2-placeId
user2-nameuser2-loginId
user3-placeIduser2-name
user3-loginIduser2-extraname
user3-nameuser3-placeId
xxuser3-loginId (graceful fail, but not data)
xxuser3-name (graceful fail, but not data)
xxuser3-extraname (graceful fail, but not data)

Really, it'd end up in the same situation as the current crash, excepted it'd be a problem for pre-8.5.1 users.

comment:21 Changed 4 years ago by jhkrischel

Some testing results:

SUCCESS:
Original client invites adium user
Adium user joins chat
Original client invites other users
Other users join chat
Adium user drops
Original client reinvites adium user
Adium user joins chat

SUCCESS:
Older adium user invites new adium user
New adium user joins chat

SUCCESS:
Adium user A invites adium user B
Adium user B joins chat

FAIL:
Adium user invites older client: 
	Meeting has already ended

FAIL:
Adium user invites older adium user
Older adium user attempts to join:
	crash

FAIL:
Older adium user invites older adium user
Older adium user attempts to join:
	crash

FAIL:
Adium user A invites adium user B
Adium user B joins chat
Adium user B invites original client:
	no invite observed

FAIL:
Adium user A invites adium user B
Adium user B joins chat
Adium user A leaves chat
Adium user B invites adium user A
Adium user A accepts chat:
	no join occurs for adium user A

FAIL:
Adium user A invites adium user B
Adium user B joins chat
Adium user A invites original client:
	meeting already ended

comment:22 in reply to: ↑ 20 ; follow-up: Changed 4 years ago by jhkrischel

Replying to jricesterenator:

Really, it'd end up in the same situation as the current crash, excepted it'd be a problem for pre-8.5.1 users.

I see what you mean - it's not enough to be not null, because the *next* guy might break you.

So a flexible patch would need to recognize what version of server we're talking to, and process differently depending on that. I'll poke around and see if there's anything obvious during the login that identifies server version...

comment:23 in reply to: ↑ 22 Changed 4 years ago by jricesterenator

Replying to jhkrischel:

Replying to jricesterenator:

Really, it'd end up in the same situation as the current crash, excepted it'd be a problem for pre-8.5.1 users.

I see what you mean - it's not enough to be not null, because the *next* guy might break you.

Exactly

So a flexible patch would need to recognize what version of server we're talking to, and process differently depending on that. I'll poke around and see if there's anything obvious during the login that identifies server version...

Thanks.

In your tests, what's the difference between "older client" and "older adium user"?

It probably makes sense that the older clients are blowing up in your tests. I imagine that the client sends an invite request to the server, and /it/ sends the official (new-style) request to the client.

We could sorta write a dynamic test case for old-style messages by intercepting the new-style message and stripping out the new field.

comment:24 Changed 4 years ago by jhkrischel

"original client" is an officially sanctioned Lotus Sametime Connect client of some sort.

"older adium user" is using 1.5.3 or prior

"adium user" or "new adium user" is using 1.5.3+patch

comment:25 Changed 4 years ago by jricesterenator

I think I've identified a control character/bytes that starts each user section. This would let us conditionally fix for new-style messages.

I'd like to confirm that someone else is getting what I'm getting. Can you put this code at the very top of your recv_SECTION_LIST() method and initiate some group chats? It'll dump the message in hex to the command line.

printf("Dumping incoming group chat invite:\n");
while(mwGetBuffer_remaining(b)) {

    guint16 val = 0;
    guint16_get(b, &val);

    printf("%04x ", val);
}
printf("\n");

If you launch Adium from the command line you should see the output, that's how it works with Pidgin at least. Or maybe you need to enable debug logging and check the debug file.

Note that this logging will prevent group chat from actually working.

dm me on Twitter for my email address, I'd like to see your messages. See previously attached "grpinvite" if you're curious what'll be in that message (pretty much just your name).

comment:26 Changed 4 years ago by jricesterenator

jhkrischel, Your in-between chars are diff than mine so we can't go that route.

I just found the server version parsing. Don't know how I missed that before.. Can you add some logging and tell me what you get? session.c:~450

For me, major ver: 0x1e, minor ver: 0x2149 (note that the minor version isn't the same thing as the mwLoginTypes in mw_common.h)

comment:27 Changed 4 years ago by jricesterenator

Note that Adium is including a meanwhile patch: http://trac.adium.im/ticket/16114#comment:27

If pidgin compiles its own depenedencies, we could go that route. Otherwise, someone'd need to be able to cross-compile for windows so we could try to get an actual patched meanwhile release. If anyone can get a cross-compile going for meanwhile, let me know.

comment:28 Changed 4 years ago by datallah

I created a patched (with the same patches that are in the adium ticket) libmeanwhile-1.dll. If someone can test it, we can include in in the next release. I don't have access to a sametime server to test it.

comment:29 follow-up: Changed 4 years ago by jricesterenator

Thanks datallah! I've tested it with a few people here at work (sametime 8.5.1), and the fix is working great.

Would you mind sharing the details of your win32 compiling setup? I spent a lot of time trying to get it going but never could. There were always glib2.0 issues or one of another many things.

comment:30 in reply to: ↑ 29 ; follow-up: Changed 4 years ago by datallah

Replying to jricesterenator:

Thanks datallah! I've tested it with a few people here at work (sametime 8.5.1), and the fix is working great.

Great. Thanks for testing.

Would you mind sharing the details of your win32 compiling setup? I spent a lot of time trying to get it going but never could. There were always glib2.0 issues or one of another many things.

Sure. It's actually somewhat of a PITA :) The setup described here is how the various win32 binaries distributed by the GTK folks are built - after jumping through a bunch of hoops to get that set up, it's relatively easy to set up a script to compile other things like this with it.

comment:31 Changed 4 years ago by Daniel Atallah <datallah@…>

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

(In [a711c88ce448]):
Patch win32 meanwhile binary with jhkrischel's adium patches to fix #12637

comment:32 Changed 4 years ago by datallah

Note that this fix only affects Windows - on other OSes, the meanwhile library is packaged by the OS, Pidgin itself has no control of patching.

comment:33 in reply to: ↑ 30 ; follow-up: Changed 4 years ago by jricesterenator

Replying to datallah:

Sure. It's actually somewhat of a PITA :) The setup described here is how the various win32 binaries distributed by the GTK folks are built - after jumping through a bunch of hoops to get that set up, it's relatively easy to set up a script to compile other things like this with it.

Thanks, this looks like exactly what I need.

datallah:

Patch win32 meanwhile binary with jhkrischel's adium patches to fix #12637

My patches ;) jhkrischel did the builds/testing.

comment:34 in reply to: ↑ 33 Changed 4 years ago by datallah

Replying to jricesterenator:

datallah:

Patch win32 meanwhile binary with jhkrischel's adium patches to fix #12637

My patches ;) jhkrischel did the builds/testing.

Oops, sorry. I can credit you in the changelog if you tell me your name.

comment:35 Changed 4 years ago by jricesterenator

Jonathan Rice

comment:36 Changed 4 years ago by Daniel Atallah <datallah@…>

(In [d1a5a89aa454]):
Credit Jonathan Rice for the libmeanwhile patch. Refs #12637

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!