GNOME Bugzilla – Bug 667866
g_thread_set_priority can set a nonsensical priority, causing a crash
Last modified: 2012-01-16 11:20:51 UTC
+++ This bug was initially created as a clone of Bug #667651 +++ Under GLib 2.28 or 2.30 (but not 2.31) on Unix, if g_thread_init() was called under a scheduling policy other than the default SCHED_OTHER, then g_thread_set_priority() can cause a fatal error. The reasoning is explained in Bug #667651. Briefly: different scheduling policies can have different allowed priority ranges. g_thread_set_priority() does not change the thread's scheduling policy (even though that has a more significant effect on the thread's priority than sched_priority does), only the sched_priority. g_thread_priority_map is populated with a mixture of sched_priority values appropriate for SCHED_OTHER, and sched_priority values appropriate for the policy used in the thread that called g_thread_init(). Depending on the priority requested and the thread's scheduling policy, this might cause pthread_setschedparam() to fail with EINVAL, which GLib treats as fatal; or it might just cause a nonsensical priority to be used (e.g. NORMAL will have higher priority than HIGH in real-time processes on Linux). This is somewhat less serious than Bug #667651 because it's easier to avoid (just don't call the function). I can see two reasonable resolutions: 1) close this bug as having been fixed in 2.31.0, and do not alter 2.30 2) make g_thread_set_priority() into a no-op in 2.30's gthread-posix.c, because at least on Linux (GLib's most widespread target platform), it never has a positive effect - it either has no effect, crashes the process, or has a negative effect (G_THREAD_PRIORITY_HIGH causing priority to be reduced) I don't see any way to make g_thread_set_priority() work as designed on POSIX without significant behaviour changes, which in any case would contradict what's happened on master for 2.32.
Created attachment 205189 [details] [review] g_thread_set_priority: make this into a no-op Previously, g_thread_set_priority() would keep the thread's current scheduling policy, but set a sched_priority from the g_thread_priority_map lookup table. This doesn't have the desired effect, because the scheduling policy has a much stronger effect on scheduling than the numeric priority; and it can also crash, because the valid values for sched_priority might vary by scheduling policy. As implemented, the LOW and URGENT ends of the lookup table are valid for SCHED_OTHER, the NORMAL value is valid for the same scheduling policy that was used in the main thread, and the HIGH value is not necessarily valid for either. This is essentially a backport of a change made by Ryan Lortie for 2.32. Bug: https://bugzilla.gnome.org/show_bug.cgi?id=667866 Bug-NB: NB#296256 Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk> --- If you want to touch this function in 2.30, here's the patch I'd use.
Attachment #205189 [details] requires Attachment #205187 [details], from Bug #667651, to have been applied first. (It removes infrastructure used by both the deleted bits of code.)
I'm of two minds. Obviously this function isn't really useful at all (which is why we removed it from master). On the other hand, I'm not sure we should go tinkering with previous stable releases like this... If people are writing new software and they find this to be a problem, then they can just not do it... If people have existing software that rely on this feature, then I don't know how I feel about yanking it out from under them in the middle of a stable cycle...
Yeah, I agree. Seems better to leave stable untouched and rejoice that the stuff is gone in master.
(In reply to comment #3) > If people have existing software > that rely on this feature, then I don't know how I feel about yanking it out > from under them in the middle of a stable cycle... Fair point. This function is basically useless, but it has *some* practical effect (even if it's often the opposite of what you want), so it seems reasonable to keep it as-is in 2.30. So, FIXED in 2.31.0. I do think Bug #667651 may be worth fixing in stable, though, since I've seen it reported independently in two distros now (Debian and Maemo), and it doesn't have the justification of having ever done something useful (because PTHREAD_INHERIT_SCHED is the default).