Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#7700 closed patch (fixed)

Zephyr with tzc can crash pidgin

Reported by: mterry Owned by: seanegan
Milestone: 2.5.3 Component: Zephyr
Version: 2.4.3 Keywords:
Cc:

Description (last modified by rekkanoryo)

  1. Start to add a zephyr account (don't have to register one anywhere, just make pidin think you have one)
  2. Make up some username
  3. In the Advanced tab, set 'Use tzc' (WITHOUT having tzc actually installed or at least have it pointing in the wrong place)
  4. Click 'Save'
  5. In the Accounts window, uncheck the account (disable it)
  6. For me, on my Ubuntu Hardy system, it crashes X (!)

I've attached a patch to improve zephyr's robustness with tzc (against Hardy's pidgin, but I checked monotone head, and the code looks the same).

It does several things:

  1. Make an exit(1) call after the execvp(). This is what caused my crash. The execvp failed (because I didn't have tzc installed), the forked child continued, and did bad things to memory.
  2. More robustly check the return value of select(). It used to be just "if (select())", now it's "if (select() > 0)", since a negative value indicates an error.
  3. Don't print debugging spew as/after we switch stdout in the forked process. The forked process's stdout is processed as tzc commands. It doesn't make sense to spit out debugging statements to ourselves, only to fail to parse it as tzc output. Better to just return -1 and fail in the unlikely scenario that close/dup2 fail.

You may notice a 10 second delay when trying to connect. This is because that's how long we wait for tzc to fail (with a select call). My patch doesn't try to correct that delay, though we could probably try to notice that the forked process exited early.

Attachments (1)

76_zephyr-handle-tzc-error.patch (2.6 KB) - added by mterry 10 years ago.
Better version, avoids delay if tzc isn't installed

Download all attachments as: .zip

Change History (7)

comment:1 Changed 10 years ago by mterry

Oh, sorry, forgot my details. I'm Michael Terry, address of michael.terry@….

Changed 10 years ago by mterry

Better version, avoids delay if tzc isn't installed

comment:2 Changed 10 years ago by mterry

I just updated the attachment to fix the select() delay.

There were two issues, depending on which side of the race condition won. If tzc ended before pidgin got to select(), select() would never return (the first select, that doesn't have a timeout). Even if pidgin did get there first, it would still wait ten seconds on the second select(). You can trigger one or the other by inserting a sleep on either side of the fork.

The second issue was easy to fix by just saving the return value of the first select(). Fixing the eternal-wait race condition was trickier. I used waitpid with the WNOHANG flag to see if tzc was still up. There is probably a cleaner way, by closing the right fd or something.

comment:3 Changed 10 years ago by rekkanoryo

  • Description modified (diff)

Fixed the formatting to be better WikiFormatting.

comment:4 Changed 10 years ago by rekkanoryo

I have credited you with the copyright of this patch; is that correct, or does Canonical hold the copyright on this patch?

comment:5 Changed 10 years ago by michael.terry@…

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

(In 5ca93c156828361226abe779294044aa16d073c5):
Fix a Zephyr crash and 10-second delay that can happen when you have an account configured to use tzc but either tzc is installed or the configured tzc path is invalid. Fixes #7700.

comment:6 Changed 10 years ago by mterry

Sorry yes, I should have mentioned, copyright Canonical. Thanks!

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!