Bug 1478 - Wrong pthread_create calls all over the place
Summary: Wrong pthread_create calls all over the place
Status: RESOLVED FIXED
Alias: None
Product: Claws Mail (GTK 2)
Classification: Unclassified
Component: Other (show other bugs)
Version: 3.2.0
Hardware: Macintosh Mac OS X 10.2
: P3 blocker
Assignee: users
URL:
Depends on:
Blocks:
 
Reported: 2008-01-20 00:50 UTC by Christian Cornelssen
Modified: 2008-01-21 11:41 UTC (History)
0 users

See Also:


Attachments
Patch that rewinds some of claws-mail's liberalization w.r.t. pthreads back to 2.10.0 usage for PPC-Mach (Mac OS X / Darwin) (2.23 KB, patch)
2008-01-20 02:18 UTC, Christian Cornelssen
no flags Details | Diff
repairs broken pthread calls (particularly useful for Darwin) (3.71 KB, patch)
2008-01-21 01:27 UTC, Christian Cornelssen
no flags Details | Diff

Description Christian Cornelssen 2008-01-20 00:50:13 UTC
My OS is MacOS X 10.4.11.
config.guess outputs "powerpc-apple-darwin8.11.0".
Installing claws-mail by means of an own Portfile for the MacPorts system.

After upgrading from claws-mail 2.10.0 (release version) to 3.2, claws-mail would exit with a bus error when requested to get new mail. Last library call turned out to be pthread_create. So I checked the changelog for changes related to pthreads.

I found this one:

2007-07-26 [colin]      2.10.0cvs67

        * src/mimeview.c
        * src/matcher.c
        * src/common/ssl.c
                Fix threads use on BSDs

My config says to use TLS when fetching mails, so I assume it's actually src/common/ssl.c. The changeset apparently makes the SSL connection become a pthread even without recent glibc, assuming use of some nice BSD libc. Sure enough, my ppc-darwin (without glibc) does not like the new code. I did not debug the code further, just refinded the #if conditions such that the change does not take effect under __MACH__ && __POWERPC__ (predefined platform macros) without glibc. This is rough obviously, but the cited changeset is rough too ;-)

The refinded #if is the following line:

#if (defined USE_PTHREAD && ((defined __GLIBC__ && (__GLIBC__ > 2 || (__GLIBC__
 == 2 && __GLIBC_MINOR__ >= 3))) || (!defined __GLIBC__ && !(defined __MACH__ &&
 defined __POWERPC__))))

This change (once in each of the mentioned files) makes claws-mail 3.2.0 run again.
Comment 1 Christian Cornelssen 2008-01-20 02:18:01 UTC
Created attachment 536 [details]
Patch that rewinds some of claws-mail's liberalization w.r.t. pthreads back to 2.10.0 usage for PPC-Mach (Mac OS X / Darwin)

The patch to 3.2.0 as described in the original bug report.
Comment 2 Colin Leroy 2008-01-20 12:04:00 UTC
It would be even better to find out the underlying reason... Could you investigate?
Comment 3 Christian Cornelssen 2008-01-20 21:50:31 UTC
> It would be even better to find out the underlying reason... Could you
> investigate?

I have taken a closer look:

src/common/ssl.c and src/matcher.c clearly violate the prototype of pthread_create() by passing PTHREAD_CREATE_JOINABLE (#defined to be 1 in my <pthread.h>) where a pthread_attr_t* is expected. I wonder why others don't seem to get bus errors / memory protection errors from this. (Perhaps the constant is 0 on other systems, and passing NULL might be valid, although my manpage does not document this.)

I suppose one has to alloc some pthread_attr_t object, then do a pthread_attr_init() on it and add a call to pthread_attr_setdetachstate() using the desired PTHREAD_CREATE_JOINABLE.

I am wondering what the precise motivation for the glibc-related conditionals has been. Could it be that glibc somehow manages to avoid bus errors (though probably not setting the desired detachstate), and so someone just thought that only glibc was suitable for his code? If so, then the conditionals could be reduced to #if USE_PTHREAD after fixing the code.

The third occurence of the glibc-related conditional is in src/mimeview.c, but the type signature of the call to pthread_create seems ok. There is even a nice call to pthread_attr_setdetachstate() using TRUE as argument, which equals PTHREAD_CREATE_JOINABLE, I suppose. (Note that the alternative PTHREAD_CREATE_DETACHED (== 2) is also nonzero, so the use of TRUE is somewhat cryptic and questionable.)

Haven't tried to clean this up yet.
Comment 4 Christian Cornelssen 2008-01-21 01:27:17 UTC
Created attachment 538 [details]
repairs broken pthread calls (particularly useful for Darwin)

I have tried to clean up the pthread mess. Offered for review here, as I am not familiar with claws-mail code. Let's call this a "plausible fix" for a bunch of obvious miscodings.

* src/common/ssl.c (SSL_connect_nb):
  - Fixed the call to pthread_create() by creating and passing an appropriate pthread_attr_t object.
  - The GNUTLS variant of the blocking (fallback) code did not return its result, thus reaching code that would wait for the nonexistent thread to terminate. Fixed.

* src/matcher.c (matcherprop_match_test):
  - Fixed the call to pthread_create() by creating and passing an appropriate pthread_attr_t object.

* src/mimeview.c (mimeview_check_sig_in_thread):
  - Use PTHREAD_CREATE_DETACHED instead of TRUE (which would mean PTHREAD_CREATE_JOINABLE at least on Darwin) in the call to pthread_attr_setdetachstate().
  - check return codes of pthread_attr_init() and pthread_attr_setdetachstate().

* src/common/ssl.c (SSL_connect_nb), src/matcher.c (matcherprop_match_test), src/mimeview.c (check_signature_cb):
  - Removed the glibc-related #if conditions near USE_PTHREAD, assuming that their only purpose was to specify the lucky cases where the broken code seemed to function.
Comment 5 Christian Cornelssen 2008-01-21 01:34:03 UTC
I should add that patch #538 compiles and works on my powerpc-apple-darwin8.11.0. At least, there have been no crashes or hangups so far.
Comment 6 Colin Leroy 2008-01-21 08:52:22 UTC
(In reply to comment #5)
>I am wondering what the precise motivation for the glibc-related conditionals
>has been. Could it be that glibc somehow manages to avoid bus errors (though
>probably not setting the desired detachstate), and so someone just thought that
>only glibc was suitable for his code?

Yes. (That was me...) Looks like this code hasn't been scrutinized enough!

>I should add that patch #538 compiles and works on my
>powerpc-apple-darwin8.11.0. At least, there have been no crashes or hangups so
>far.

Applying, thanks a bunch.

Comment 7 users 2008-01-21 08:55:44 UTC
Changes related to this bug have been committed.
Please check latest CVS and update the bug accordingly.
You can also get the patch from:
http://www.colino.net/claws-mail/

2008-01-21 [colin]	3.2.0cvs57

	* AUTHORS
	* src/matcher.c
	* src/mimeview.c
	* src/common/ssl.c
	* src/gtk/authors.h
		Fix bug 1478, 'Wrong pthread_create calls all 
		over the place'. Patch by Christian Cornelssen

Note You need to log in before you can comment on or make changes to this bug.