May 2006 - Freeradius-Devel - lists.freeradius.org

Re: delete_thread() no mutex?
by Frank Cusack 10 May '06

10 May '06

On May 10, 2006 12:14:24 AM -0700 Frank Cusack <fcusack(a)fcusack.com> wrote: > I was wondering why threads.c:thread_pool_clean() doesn't need a mutex > when calling delete_thread(). > > There is a comment just below that, that a mutex isn't needed to read > "the location" -- which I take to mean thread_pool.active_threads -- > as it's only needed to modify the location. > > However, just below that, spawn_thread() is called, which does modify > thread_pool.active_threads and a mutex isn't held to do that. > >> From a quick read (and late at night), ISTM that this is just an error > in documentation. > > a) Only the main thread deals with active_threads and total_threads > (this is a rough guess, not thoroughly explored) > b) There isn't even a mutex to be locked! thread_pool has 2 mutexes, > they are used for different things. > > Sound about right? Or is there indeed some missing locking. After some more study, I'm a little bit off in my explanation. This is the kind of thing that's way easier on a whiteboard, but anyway ... spawn_thread() modifies total_threads, not active_threads, so this is ok. (AFAICT total_threads is only used by the main thread.) active_threads is protected by the queue mutex (should probably be better documented), and in that case I agree with the comment in t_p_clean(). Alan or someone please confirm or argue. But I have a new question. thread_pool_addrequest() uses active_threads to determine if it should spawn, but it doesn't hold the queue mutex. So it could err either way. I'd argue that the log message can be bogus but if you're that much against the limit it doesn't matter. There should just be a comment there about possible stale value. And one last question. Why does total_active_threads walk the entire thread pool? It should just return active_threads. Either way it can return a stale value. I'd go ahead and make those changes but that's hairy part of the server so my confidence about being correct isn't super high. -frank

2 1

Automatic report from sources (radiusd) between 09.05.2006 - 10.05.2006 GMT
by Automatic cvs log generator 10 May '06

10 May '06

CVS log entries from 09.05.2006 (Tue) 08:00:01 - 10.05.2006 (Wed) 08:00:01 GMT ===================================================== Summary by authors ===================================================== Author: aland File: radiusd/doc/ChangeLog; Revisions: 1.57.2.31.2.18 File: radiusd/src/modules/rlm_eap/mem.c; Revisions: 1.18, 1.14.4.2 Author: fcusack File: radiusd/src/modules/rlm_otp/cardops/cryptocard.h; Revisions: 1.40, 1.37.2.3 File: radiusd/src/modules/rlm_otp/otp_cardops.h; Revisions: 1.44, 1.41.2.3 File: radiusd/src/main/threads.c; Revisions: 1.106 File: radiusd/src/modules/rlm_otp/cardops/cryptocard.c; Revisions: 1.42, 1.39.2.3 File: radiusd/src/modules/rlm_otp/otp_cardops.c; Revisions: 1.65, 1.62.2.3 Author: pnixon File: radiusd/doc/examples/postgresql.sql; Revisions: 1.5 ===================================================== Combined list of identical log entries ===================================================== Description: import from HEAD: update cardops keystring2keyblock method to take a card_info_t * instead of some of its fields Modified files: File: radiusd/src/modules/rlm_otp/otp_cardops.c; Revision: 1.62.2.3; Date: 2006/05/09 08:13:03; Author: fcusack; Lines: (+4 -5) File: radiusd/src/modules/rlm_otp/otp_cardops.h; Revision: 1.41.2.3; Date: 2006/05/09 08:13:04; Author: fcusack; Lines: (+3 -3) File: radiusd/src/modules/rlm_otp/cardops/cryptocard.c; Revision: 1.39.2.3; Date: 2006/05/09 08:13:04; Author: fcusack; Lines: (+5 -6) File: radiusd/src/modules/rlm_otp/cardops/cryptocard.h; Revision: 1.37.2.3; Date: 2006/05/09 08:13:05; Author: fcusack; Lines: (+3 -4) ------------------------------- Description: update cardops keystring2keyblock method to take a card_info_t * instead of some of its fields Modified files: File: radiusd/src/modules/rlm_otp/otp_cardops.c; Revision: 1.65; Date: 2006/05/09 08:08:23; Author: fcusack; Lines: (+3 -4) File: radiusd/src/modules/rlm_otp/otp_cardops.h; Revision: 1.44; Date: 2006/05/09 08:08:23; Author: fcusack; Lines: (+2 -2) File: radiusd/src/modules/rlm_otp/cardops/cryptocard.c; Revision: 1.42; Date: 2006/05/09 08:08:23; Author: fcusack; Lines: (+4 -5) File: radiusd/src/modules/rlm_otp/cardops/cryptocard.h; Revision: 1.40; Date: 2006/05/09 08:08:23; Author: fcusack; Lines: (+2 -3) ------------------------------- Description: Fix linked list code for EAP handlers. Modified files: File: radiusd/src/modules/rlm_eap/mem.c; Revision: 1.18; Date: 2006/05/09 20:59:55; Author: aland; Lines: (+7 -4) File: radiusd/src/modules/rlm_eap/mem.c; Revision: 1.14.4.2; Date: 2006/05/09 20:59:23; Author: aland; Lines: (+7 -4) ===================================================== Log entries ===================================================== Description: Updates Modified files: File: radiusd/doc/ChangeLog; Revision: 1.57.2.31.2.18; Date: 2006/05/09 23:14:33; Author: aland; Lines: (+9 -6) ------------------------------- Description: Add Called/Calling StationId Modified files: File: radiusd/doc/examples/postgresql.sql; Revision: 1.5; Date: 2006/05/09 11:47:10; Author: pnixon; Lines: (+9 -7) ------------------------------- Description: delete_thread(): clarify thread exit comments Modified files: File: radiusd/src/main/threads.c; Revision: 1.106; Date: 2006/05/10 06:40:54; Author: fcusack; Lines: (+9 -13) ===================================================== Summary of modified files ===================================================== File: radiusd/doc/ChangeLog Revisions: 1.57.2.31.2.18 Authors: aland (+9 -6) ------------------------------- File: radiusd/doc/examples/postgresql.sql Revisions: 1.5 Authors: pnixon (+9 -7) ------------------------------- File: radiusd/src/main/threads.c Revisions: 1.106 Authors: fcusack (+9 -13) ------------------------------- File: radiusd/src/modules/rlm_eap/mem.c Revisions: 1.18, 1.14.4.2 Authors: aland (+7 -4), aland (+7 -4) ------------------------------- File: radiusd/src/modules/rlm_otp/cardops/cryptocard.c Revisions: 1.42, 1.39.2.3 Authors: fcusack (+4 -5), fcusack (+5 -6) ------------------------------- File: radiusd/src/modules/rlm_otp/cardops/cryptocard.h Revisions: 1.40, 1.37.2.3 Authors: fcusack (+2 -3), fcusack (+3 -4) ------------------------------- File: radiusd/src/modules/rlm_otp/otp_cardops.c Revisions: 1.65, 1.62.2.3 Authors: fcusack (+3 -4), fcusack (+4 -5) ------------------------------- File: radiusd/src/modules/rlm_otp/otp_cardops.h Revisions: 1.44, 1.41.2.3 Authors: fcusack (+2 -2), fcusack (+3 -3) -- Automatic cron job from /web/pages/us.freeradius.org/bin/new_makelog.pl

1 0

delete_thread() no mutex?
by Frank Cusack 10 May '06

10 May '06

I was wondering why threads.c:thread_pool_clean() doesn't need a mutex when calling delete_thread(). There is a comment just below that, that a mutex isn't needed to read "the location" -- which I take to mean thread_pool.active_threads -- as it's only needed to modify the location. However, just below that, spawn_thread() is called, which does modify thread_pool.active_threads and a mutex isn't held to do that. >From a quick read (and late at night), ISTM that this is just an error in documentation. a) Only the main thread deals with active_threads and total_threads (this is a rough guess, not thoroughly explored) b) There isn't even a mutex to be locked! thread_pool has 2 mutexes, they are used for different things. Sound about right? Or is there indeed some missing locking. -frank

1 0

Automatic report from sources (radiusd) between 08.05.2006 - 09.05.2006 GMT
by Automatic cvs log generator 09 May '06

09 May '06

CVS log entries from 08.05.2006 (Mon) 08:00:01 - 09.05.2006 (Tue) 08:00:01 GMT ===================================================== Summary by authors ===================================================== Author: aland File: radiusd/src/modules/rlm_ldap/rlm_ldap.c; Revisions: 1.171, 1.122.2.8.2.7 ===================================================== Combined list of identical log entries ===================================================== Description: Initialize flags. Modified files: File: radiusd/src/modules/rlm_ldap/rlm_ldap.c; Revision: 1.171; Date: 2006/05/08 23:45:49; Author: aland; Lines: (+2 -1) File: radiusd/src/modules/rlm_ldap/rlm_ldap.c; Revision: 1.122.2.8.2.7; Date: 2006/05/08 23:45:02; Author: aland; Lines: (+2 -1) ===================================================== Log entries ===================================================== ===================================================== Summary of modified files ===================================================== File: radiusd/src/modules/rlm_ldap/rlm_ldap.c Revisions: 1.171, 1.122.2.8.2.7 Authors: aland (+2 -1), aland (+2 -1) -- Automatic cron job from /web/pages/us.freeradius.org/bin/new_makelog.pl

1 0

Automatic report from sources (radiusd) between 07.05.2006 - 08.05.2006 GMT
by Automatic cvs log generator 08 May '06

08 May '06

CVS log entries from 07.05.2006 (Sun) 08:00:01 - 08.05.2006 (Mon) 08:00:01 GMT ===================================================== Summary by authors ===================================================== Author: pnixon File: radiusd/doc/examples/postgresql.sql; Revisions: 1.4 ===================================================== Log entries ===================================================== Description: This index modification makes accounting_update_query and accounting_stop_query several orders of magnitude faster at the expense of slowing down accounting_start_query a bit which overall makes the postgresql backend 200-300 times faster when using the defaul queries and schema with a "reasonable" amount of data. Modified files: File: radiusd/doc/examples/postgresql.sql; Revision: 1.4; Date: 2006/05/07 21:37:23; Author: pnixon; Lines: (+3 -3) ===================================================== Summary of modified files ===================================================== File: radiusd/doc/examples/postgresql.sql Revisions: 1.4 Authors: pnixon (+3 -3) -- Automatic cron job from /web/pages/us.freeradius.org/bin/new_makelog.pl

1 0

Automatic report from sources (radiusd) between 05.05.2006 - 06.05.2006 GMT
by Automatic cvs log generator 06 May '06

06 May '06

CVS log entries from 05.05.2006 (Fri) 08:00:01 - 06.05.2006 (Sat) 08:00:01 GMT ===================================================== Summary by authors ===================================================== Author: aland File: radiusd/src/modules/rlm_preprocess/rlm_preprocess.c; Revisions: 1.67, 1.52.2.1.2.1 ===================================================== Combined list of identical log entries ===================================================== Description: Don't de-reference request->username if it's NULL Modified files: File: radiusd/src/modules/rlm_preprocess/rlm_preprocess.c; Revision: 1.67; Date: 2006/05/05 17:31:28; Author: aland; Lines: (+4 -4) File: radiusd/src/modules/rlm_preprocess/rlm_preprocess.c; Revision: 1.52.2.1.2.1; Date: 2006/05/05 17:31:53; Author: aland; Lines: (+4 -4) ===================================================== Log entries ===================================================== ===================================================== Summary of modified files ===================================================== File: radiusd/src/modules/rlm_preprocess/rlm_preprocess.c Revisions: 1.67, 1.52.2.1.2.1 Authors: aland (+4 -4), aland (+4 -4) -- Automatic cron job from /web/pages/us.freeradius.org/bin/new_makelog.pl

1 0

Automatic report from sources (radiusd) between 03.05.2006 - 04.05.2006 GMT
by Automatic cvs log generator 04 May '06

04 May '06

CVS log entries from 03.05.2006 (Wed) 08:00:02 - 04.05.2006 (Thu) 08:00:01 GMT ===================================================== Summary by authors ===================================================== Author: aland File: radiusd/share/dictionary.erx; Revisions: 1.14, 1.9.4.3 File: radiusd/doc/ChangeLog; Revisions: 1.57.2.31.2.17 File: radiusd/src/lib/radius.c; Revisions: 1.182, 1.125.2.5.2.6 File: radiusd/src/lib/valuepair.c; Revisions: 1.106, 1.82.2.1.2.3, 1.82.2.1.2.2 File: radiusd/src/lib/filters.c; Revisions: 1.47, 1.46, 1.36.4.3, 1.36.4.2 ===================================================== Combined list of identical log entries ===================================================== Description: Whoops.. don't make abinary into octets when we receive it. Modified files: File: radiusd/src/lib/radius.c; Revision: 1.182; Date: 2006/05/03 17:39:12; Author: aland; Lines: (+3 -2) File: radiusd/src/lib/radius.c; Revision: 1.125.2.5.2.6; Date: 2006/05/03 17:39:37; Author: aland; Lines: (+3 -2) ------------------------------- Description: Better printing of octets Modified files: File: radiusd/src/lib/filters.c; Revision: 1.47; Date: 2006/05/03 17:34:48; Author: aland; Lines: (+6 -4) File: radiusd/src/lib/filters.c; Revision: 1.36.4.3; Date: 2006/05/03 17:35:02; Author: aland; Lines: (+6 -4) ------------------------------- Description: Note abinary support Modified files: File: radiusd/share/dictionary.erx; Revision: 1.14; Date: 2006/05/03 17:46:57; Author: aland; Lines: (+9 -1) File: radiusd/share/dictionary.erx; Revision: 1.9.4.3; Date: 2006/05/03 17:46:42; Author: aland; Lines: (+9 -1) ===================================================== Log entries ===================================================== Description: Note recent changes Modified files: File: radiusd/doc/ChangeLog; Revision: 1.57.2.31.2.17; Date: 2006/05/03 17:42:49; Author: aland; Lines: (+9 -3) ------------------------------- Description: Initialize error string to something sane use sizeof(*ptr) rather than sizeof(ptr) Modified files: File: radiusd/src/lib/filters.c; Revision: 1.46; Date: 2006/05/03 17:23:45; Author: aland; Lines: (+10 -3) ------------------------------- Description: Initialize error string to something sane Modified files: File: radiusd/src/lib/filters.c; Revision: 1.36.4.2; Date: 2006/05/03 17:18:25; Author: aland; Lines: (+10 -3) ------------------------------- Description: Allow 0x... for abinary Modified files: File: radiusd/src/lib/valuepair.c; Revision: 1.106; Date: 2006/05/03 17:38:51; Author: aland; Lines: (+10 -7) ------------------------------- Description: If abinary is 0x..., then make it type "octets" Modified files: File: radiusd/src/lib/valuepair.c; Revision: 1.82.2.1.2.3; Date: 2006/05/03 17:40:58; Author: aland; Lines: (+6 -4) ------------------------------- Description: Allow hex data in abinary attributes, to be more flexible. Return error from pairread() if we failed to create an attribute. This will break existing configs that refer to non-existent attributes, BUT there will now be a useful error message where there wasn't anything before. Modified files: File: radiusd/src/lib/valuepair.c; Revision: 1.82.2.1.2.2; Date: 2006/05/03 17:18:09; Author: aland; Lines: (+17 -3) ===================================================== Summary of modified files ===================================================== File: radiusd/doc/ChangeLog Revisions: 1.57.2.31.2.17 Authors: aland (+9 -3) ------------------------------- File: radiusd/share/dictionary.erx Revisions: 1.14, 1.9.4.3 Authors: aland (+9 -1), aland (+9 -1) ------------------------------- File: radiusd/src/lib/filters.c Revisions: 1.47, 1.46, 1.36.4.3, 1.36.4.2 Authors: aland (+6 -4), aland (+10 -3), aland (+6 -4), aland (+10 -3) ------------------------------- File: radiusd/src/lib/radius.c Revisions: 1.182, 1.125.2.5.2.6 Authors: aland (+3 -2), aland (+3 -2) ------------------------------- File: radiusd/src/lib/valuepair.c Revisions: 1.106, 1.82.2.1.2.3, 1.82.2.1.2.2 Authors: aland (+10 -7), aland (+6 -4), aland (+17 -3) -- Automatic cron job from /web/pages/us.freeradius.org/bin/new_makelog.pl

1 0

lib/filters.c returning incorrect length filters
by Mitchell, Michael J 03 May '06

03 May '06

Hi freeRADIUS developers, Firstly, a disclaimer/confession. The analysis below is based on freeRADIUS version 1.0.1, however I can't see any significant differences between this and CVS HEAD in relation to this issue. The patch I have attached is based on CVS HEAD, but is unfortunately untested. Please feel free to tell me its not an issue in CVS HEAD any more due to other changes, or to go away and test it first, but with my current work load I'm not sure when I will be able to do that. While testing Ascend Data Filters (IP filters) it became apparent that freeRADIUS was sending 32 byte long Ascend-Data-Filter attributes. In our case our NAS's expect 24 byte IP filters (while some newer NAS's apparently expect 26 bytes?) and complains when its gets something longer. Taking a look at lib/filters.c it became apparent that the code makes no distinction between the various filter types when assigning the length of the attribute. The attached patch helps address this issue by assigning the length of the string (or vp_filter) attribute based on the actual size of the filter type (and corresponding changes for printing Ascend Data Filter attributes). A bit of reading on the web revealed that some other RADIUS servers allow the fill size at the end of an IP filter to be configured (to allow for 24 or 26 byte filters more easily). This looks a bit difficult in freeRADIUS due to the code being in libradius rather than the core. It is also something that would need to be configured on a per client basis, which is rather nasty. I believe there is also a bug in the print_abinary() function. It was comparing the vp->length to sizeof(filter), but filter is now a pointer. Also, it is not possible to do a "!=" comparison here, as different filter types have different lengths. At worst we can do a ">" comparison, a better solution would be to do the comparison based on the actual filter type further down the code. My patch compares the length of the attribute against the filter type. regards, Mike

4 6

Automatic report from sources (radiusd) between 02.05.2006 - 03.05.2006 GMT
by Automatic cvs log generator 03 May '06

03 May '06

CVS log entries from 02.05.2006 (Tue) 08:00:01 - 03.05.2006 (Wed) 08:00:02 GMT ===================================================== Summary by authors ===================================================== Author: aland File: radiusd/src/lib/radius.c; Revisions: 1.181 File: radiusd/src/main/radiusd.c; Revisions: 1.356 File: radiusd/src/lib/print.c; Revisions: 1.41 ===================================================== Log entries ===================================================== Description: signed/unsigned fixes Modified files: File: radiusd/src/lib/print.c; Revision: 1.41; Date: 2006/05/02 21:06:11; Author: aland; Lines: (+5 -5) ------------------------------- Description: cleaner random stuff Modified files: File: radiusd/src/lib/radius.c; Revision: 1.181; Date: 2006/05/02 21:07:30; Author: aland; Lines: (+14 -29) ------------------------------- Description: mainconfig.radiusd_conf is dynamically allocated, not static Modified files: File: radiusd/src/main/radiusd.c; Revision: 1.356; Date: 2006/05/02 21:47:46; Author: aland; Lines: (+4 -3) ===================================================== Summary of modified files ===================================================== File: radiusd/src/lib/print.c Revisions: 1.41 Authors: aland (+5 -5) ------------------------------- File: radiusd/src/lib/radius.c Revisions: 1.181 Authors: aland (+14 -29) ------------------------------- File: radiusd/src/main/radiusd.c Revisions: 1.356 Authors: aland (+4 -3) -- Automatic cron job from /web/pages/us.freeradius.org/bin/new_makelog.pl

1 0

Null pointer deref
by Daniel O'Connor 02 May '06

02 May '06

Hi, I am trying to get FreeRadius running on FreeBSD to handle WPA from an OpenWRT box, but I found that FreeRadius would crash when a request was processed. The strcmp's around lines 1630 and 1659 can be done on a NULL pointer (eg mainconfig.do_lower_user) which causes a crash. It is trivial to add a 'mainconfig.do_lower_user != NULL && ' (or similar) in front of these strcmps. -- Daniel O'Connor software and network engineer for Genesis Software - http://www.gsoft.com.au "The nice thing about standards is that there are so many of them to choose from." -- Andrew Tanenbaum GPG Fingerprint - 5596 B766 97C0 0E94 4347 295E E593 DC20 7B3F CE8C

2 6

Automatic report from sources (radiusd) between 01.05.2006 - 02.05.2006 GMT
by Automatic cvs log generator 02 May '06

02 May '06

CVS log entries from 01.05.2006 (Mon) 08:00:01 - 02.05.2006 (Tue) 08:00:01 GMT ===================================================== Summary by authors ===================================================== Author: aland File: radiusd/src/modules/rlm_eap/libeap/mppe_keys.c; Revisions: 1.3 File: radiusd/src/modules/rlm_eap/libeap/cb.c; Revisions: 1.4, 1.1.2.3 File: radiusd/src/modules/rlm_eap/libeap/eapsimlib.c; Revisions: 1.14 File: radiusd/src/modules/rlm_eap/libeap/eapcommon.c; Revisions: 1.10 ===================================================== Combined list of identical log entries ===================================================== Description: Signed/unsigned fixes Modified files: File: radiusd/src/modules/rlm_eap/libeap/eapcommon.c; Revision: 1.10; Date: 2006/05/01 16:47:11; Author: aland; Lines: (+3 -3) File: radiusd/src/modules/rlm_eap/libeap/eapsimlib.c; Revision: 1.14; Date: 2006/05/01 16:47:10; Author: aland; Lines: (+5 -5) File: radiusd/src/modules/rlm_eap/libeap/mppe_keys.c; Revision: 1.3; Date: 2006/05/01 16:47:10; Author: aland; Lines: (+3 -3) ===================================================== Log entries ===================================================== Description: Catch NULL arg and return. This fixes bug #298 Modified files: File: radiusd/src/modules/rlm_eap/libeap/cb.c; Revision: 1.4; Date: 2006/05/01 16:47:47; Author: aland; Lines: (+7 -1) ------------------------------- Description: Pull fix for #298 from the head Modified files: File: radiusd/src/modules/rlm_eap/libeap/cb.c; Revision: 1.1.2.3; Date: 2006/05/01 16:48:11; Author: aland; Lines: (+7 -1) ===================================================== Summary of modified files ===================================================== File: radiusd/src/modules/rlm_eap/libeap/cb.c Revisions: 1.4, 1.1.2.3 Authors: aland (+7 -1), aland (+7 -1) ------------------------------- File: radiusd/src/modules/rlm_eap/libeap/eapcommon.c Revisions: 1.10 Authors: aland (+3 -3) ------------------------------- File: radiusd/src/modules/rlm_eap/libeap/eapsimlib.c Revisions: 1.14 Authors: aland (+5 -5) ------------------------------- File: radiusd/src/modules/rlm_eap/libeap/mppe_keys.c Revisions: 1.3 Authors: aland (+3 -3) -- Automatic cron job from /web/pages/us.freeradius.org/bin/new_makelog.pl

1 0