Hello! I just noticed something strange with 3.0.21 from buster-backports. I have a custom policy in policy.d/thm_rada uses another policy from policy.d/canonicalization and this works in my test system: [...] including files in directory /etc/freeradius/3.0/policy.d/ including configuration file /etc/freeradius/3.0/policy.d/control including configuration file /etc/freeradius/3.0/policy.d/rfc7542 including configuration file /etc/freeradius/3.0/policy.d/canonicalization including configuration file /etc/freeradius/3.0/policy.d/abfab-tr including configuration file /etc/freeradius/3.0/policy.d/dhcp including configuration file /etc/freeradius/3.0/policy.d/thm_rada including configuration file /etc/freeradius/3.0/policy.d/debug including configuration file /etc/freeradius/3.0/policy.d/accounting including configuration file /etc/freeradius/3.0/policy.d/eap including configuration file /etc/freeradius/3.0/policy.d/cui including configuration file /etc/freeradius/3.0/policy.d/filter including configuration file /etc/freeradius/3.0/policy.d/moonshot-targeted-ids including configuration file /etc/freeradius/3.0/policy.d/operator-name including files in directory /etc/freeradius/3.0/sites-enabled/ including configuration file /etc/freeradius/3.0/sites-enabled/default main { security { user = "freerad" group = "freerad" [...] Now I tar'ed and copied the whole /etc/freeradius/ structure to my new to-be-production system and this happens: [...] including files in directory /etc/freeradius/3.0/policy.d/ including configuration file /etc/freeradius/3.0/policy.d/eap including configuration file /etc/freeradius/3.0/policy.d/control including configuration file /etc/freeradius/3.0/policy.d/operator-name including configuration file /etc/freeradius/3.0/policy.d/debug including configuration file /etc/freeradius/3.0/policy.d/abfab-tr including configuration file /etc/freeradius/3.0/policy.d/thm_rada /etc/freeradius/3.0/policy.d/thm_rada[6]: Reference "${policy.mac-addr-regexp}" not found /etc/freeradius/3.0/policy.d/thm_rada[6]: Parse error expanding ${...} in condition Errors reading or parsing /etc/freeradius/3.0/radiusd.conf [...] Notice how the load-order is different? It seems freeradius does not load the files in alphanumeric order but in the order the filesystem presents them, breaking the parsing of policy.d/thm_rada in this case because policy.d/canonicalization hasn't been parsed yet. Is this known or am I doing something wrong? The comment states "If policy A calls policy B, then B MUST be defined before A." which I am doing here, because "t" comes after "c" but it clearly isn't doing that. Grüße, Sven
On 15.07.20 13:51, Sven Hartge wrote:
Hello!
I just noticed something strange with 3.0.21 from buster-backports.
I have a custom policy in policy.d/thm_rada uses another policy from policy.d/canonicalization and this works in my test system:
[...] including files in directory /etc/freeradius/3.0/policy.d/ including configuration file /etc/freeradius/3.0/policy.d/control including configuration file /etc/freeradius/3.0/policy.d/rfc7542 including configuration file /etc/freeradius/3.0/policy.d/canonicalization including configuration file /etc/freeradius/3.0/policy.d/abfab-tr including configuration file /etc/freeradius/3.0/policy.d/dhcp including configuration file /etc/freeradius/3.0/policy.d/thm_rada including configuration file /etc/freeradius/3.0/policy.d/debug including configuration file /etc/freeradius/3.0/policy.d/accounting including configuration file /etc/freeradius/3.0/policy.d/eap including configuration file /etc/freeradius/3.0/policy.d/cui including configuration file /etc/freeradius/3.0/policy.d/filter including configuration file /etc/freeradius/3.0/policy.d/moonshot-targeted-ids including configuration file /etc/freeradius/3.0/policy.d/operator-name including files in directory /etc/freeradius/3.0/sites-enabled/ including configuration file /etc/freeradius/3.0/sites-enabled/default main { security { user = "freerad" group = "freerad" [...]
Now I tar'ed and copied the whole /etc/freeradius/ structure to my new to-be-production system and this happens:
[...] including files in directory /etc/freeradius/3.0/policy.d/ including configuration file /etc/freeradius/3.0/policy.d/eap including configuration file /etc/freeradius/3.0/policy.d/control including configuration file /etc/freeradius/3.0/policy.d/operator-name including configuration file /etc/freeradius/3.0/policy.d/debug including configuration file /etc/freeradius/3.0/policy.d/abfab-tr including configuration file /etc/freeradius/3.0/policy.d/thm_rada /etc/freeradius/3.0/policy.d/thm_rada[6]: Reference "${policy.mac-addr-regexp}" not found /etc/freeradius/3.0/policy.d/thm_rada[6]: Parse error expanding ${...} in condition Errors reading or parsing /etc/freeradius/3.0/radiusd.conf [...]
Notice how the load-order is different? It seems freeradius does not load the files in alphanumeric order but in the order the filesystem presents them, breaking the parsing of policy.d/thm_rada in this case because policy.d/canonicalization hasn't been parsed yet.
Adding to that: On the working system the output of "ls -fl" shows the file/dirctory order as follows: -rw-r----- 1 freerad freerad 581 Jan 29 08:43 control -rw-r----- 1 freerad freerad 1610 Jan 29 08:43 rfc7542 -rw-r----- 1 freerad freerad 2704 Jan 29 09:29 canonicalization -rw-r----- 1 freerad freerad 2787 Jan 29 08:43 abfab-tr -rw-r----- 1 freerad freerad 644 Jan 29 08:43 dhcp -rw-r--r-- 1 freerad freerad 564 Jan 29 09:30 thm_rada -rw-r----- 1 freerad freerad 1010 Jan 29 08:43 debug -rw-r----- 1 freerad freerad 4202 Jan 29 08:43 accounting -rw-r----- 1 freerad freerad 1320 Jan 29 08:43 eap drwxr-xr-x 9 freerad freerad 4096 Apr 23 10:58 .. -rw-r----- 1 freerad freerad 3847 Jan 29 08:43 cui -rw-r----- 1 freerad freerad 4746 Jan 29 08:43 filter drwxr-xr-x 2 freerad freerad 4096 Apr 23 10:58 . -rw-r----- 1 freerad freerad 8201 Jan 29 08:43 moonshot-targeted-ids -rw-r----- 1 freerad freerad 1323 Jan 29 08:43 operator-name And this matches the load order in freeradius. And on the non-working system it is: -rw-r----- 1 freerad freerad 1320 Jul 15 14:03 eap -rw-r----- 1 freerad freerad 581 Jul 15 14:03 control -rw-r----- 1 freerad freerad 1323 Jul 15 14:03 operator-name -rw-r----- 1 freerad freerad 1010 Jul 15 14:03 debug drwxr-xr-x 2 freerad freerad 4096 Jul 15 14:03 . -rw-r----- 1 freerad freerad 2787 Jul 15 14:03 abfab-tr -rw-r--r-- 1 freerad freerad 564 Jul 15 14:03 thm_rada -rw-r----- 1 freerad freerad 3847 Jul 15 14:03 cui -rw-r----- 1 freerad freerad 4746 Jul 15 14:03 filter -rw-r----- 1 freerad freerad 4202 Jul 15 14:03 accounting drwxr-xr-x 9 freerad freerad 4096 Jul 15 13:53 .. -rw-r----- 1 freerad freerad 8201 Jul 15 14:03 moonshot-targeted-ids -rw-r----- 1 freerad freerad 2704 Jul 15 14:03 canonicalization -rw-r----- 1 freerad freerad 1610 Jul 15 14:03 rfc7542 -rw-r----- 1 freerad freerad 644 Jul 15 14:03 dhcp Also interesting: If I copy the files away and then copy them back, the same order persists, so there is at least some stability here. It just doesn't change the fact that this breaks freeradius for me, unless I manually finagle the file order in the directory every time I make a change. Grüße, Sven.
On 15.07.20 14:07, Sven Hartge wrote:
It just doesn't change the fact that this breaks freeradius for me, unless I manually finagle the file order in the directory every time I make a change.
From my limited C-knowledge it seems to me the problem is rooted in the readdir() call in freeradius/src/main/conffile.c:2512 readdir() does not guarantee any sort of order of the dirents read from dirp, leading to this problem here. Grüße, Sven.
On 15/07/2020 12:51, Sven Hartge wrote:> I just noticed something strange with 3.0.21 from buster-backport...> Notice how the load-order is different? It seems freeradius does not > load the files in alphanumeric order but in the order the filesystem > presents them, breaking the parsing of policy.d/thm_rada in this case > because policy.d/canonicalization hasn't been parsed yet.> > Is this known or am I doing something wrong? I don't believe FreeRADIUS has ever made any guarantees about file load order in a directory.
The comment states "If policy A calls policy B, then B MUST be defined before A." which I am doing here, because "t" comes after "c" but it clearly isn't doing that.
You can include a file in the main radiusd.conf which contains your definitions, before the policy directory is included. That way it's explicit in the config and won't break. -- Matthew
On 15.07.20 14:13, Matthew Newton wrote:
On 15/07/2020 12:51, Sven Hartge wrote:> I just noticed something strange with 3.0.21 from buster-backport...> Notice how the load-order is different? It seems freeradius does not > load the files in alphanumeric order but in the order the filesystem > presents them, breaking the parsing of policy.d/thm_rada in this case > because policy.d/canonicalization hasn't been parsed yet.> > Is this known or am I doing something wrong? I don't believe FreeRADIUS has ever made any guarantees about file load order in a directory.
The comment states "If policy A calls policy B, then B MUST be defined before A." which I am doing here, because "t" comes after "c" but it clearly isn't doing that.
You can include a file in the main radiusd.conf which contains your definitions, before the policy directory is included. That way it's explicit in the config and won't break.
Of course, as a quick workaround I could also just append the contents of thm_rada to canonicalization to make it work. But I'd rather keep the configuration as pristine as possible to make upgrades down the line easier. And in the end the load order should be fixed by sorting before loading to make it stable for everyone and not break semi-randomly based on the way a filesystem presents/stores the entries in a directory. Grüße, Sven.
On Jul 15, 2020, at 8:19 AM, Sven Hartge <sven@svenhartge.de> wrote:
And in the end the load order should be fixed by sorting before loading to make it stable for everyone and not break semi-randomly based on the way a filesystem presents/stores the entries in a directory.
As alway, patches are welcome. Alan DeKok.
On 15.07.20 17:14, Alan DeKok wrote:
On Jul 15, 2020, at 8:19 AM, Sven Hartge <sven@svenhartge.de> wrote:
And in the end the load order should be fixed by sorting before loading to make it stable for everyone and not break semi-randomly based on the way a filesystem presents/stores the entries in a directory.
As alway, patches are welcome.
I really would, but unfortunately I only know enough C to be a danger to myself. A little man-page reading points me to scandir() instead of opendir()/readdir() but I have no idea how portable that is but since you already check for dirent.h it should be safe to use. Also string pointers in C give me the creeps, sorry. Grüße, Sven.
On 15.07.20 17:14, Alan DeKok wrote:
On Jul 15, 2020, at 8:19 AM, Sven Hartge <sven@svenhartge.de> wrote:
And in the end the load order should be fixed by sorting before loading to make it stable for everyone and not break semi-randomly based on the way a filesystem presents/stores the entries in a directory.
As alway, patches are welcome.
OK, please see the attached diff. Please note that I have no idea what I am doing, this may be memleaky as hell (although I think I added the free(namelist) to the correct places) and is not tested in any way. I just adapted the example code from the scandir(3) man-page and massaged it a bit to fit the existing code. Also note: if one uses scandir one could technically use its own filter to remove '.' and '..' from the list of entries but I wanted to change to be as minimal as possible. Grüße, Sven.
On Jul 15, 2020, at 11:49 AM, Sven Hartge <sven@svenhartge.de> wrote:
OK, please see the attached diff.
Nothing was attached. :(
Please note that I have no idea what I am doing, this may be memleaky as hell (although I think I added the free(namelist) to the correct places) and is not tested in any way.
I just adapted the example code from the scandir(3) man-page and massaged it a bit to fit the existing code.
Also note: if one uses scandir one could technically use its own filter to remove '.' and '..' from the list of entries but I wanted to change to be as minimal as possible.
Sure. The larger issue is that I tested it on OSX, and scandir() returns *duplicates*. i.e. it seems to randomly return the same file multiple times. So that's useless. Quite frankly, it's not a lot more code to just readdir() the entire directory, and put the files into an ordered heap. The "read file" code can then just pop entries from the heap. Alan DeKok.
On 15.07.20 17:54, Alan DeKok wrote:
On Jul 15, 2020, at 11:49 AM, Sven Hartge <sven@svenhartge.de> wrote:
OK, please see the attached diff.
Nothing was attached. :(
Yes, the listmanager ate it. See my other mail. But based on your tests with scandir() it will not be useful.
Please note that I have no idea what I am doing, this may be memleaky as hell (although I think I added the free(namelist) to the correct places) and is not tested in any way.
I just adapted the example code from the scandir(3) man-page and massaged it a bit to fit the existing code.
Also note: if one uses scandir one could technically use its own filter to remove '.' and '..' from the list of entries but I wanted to change to be as minimal as possible.
Sure. The larger issue is that I tested it on OSX, and scandir() returns *duplicates*. i.e. it seems to randomly return the same file multiple times. So that's useless.
What? That is beyond useless, that is broken.
Quite frankly, it's not a lot more code to just readdir() the entire directory, and put the files into an ordered heap. The "read file" code can then just pop entries from the heap.
Well, that here is the moment I step of the train, that is way past my abilities, sorry. Grüße, Sven.
On 15.07.20 17:54, Alan DeKok wrote:
Sure. The larger issue is that I tested it on OSX, and scandir() returns *duplicates*. i.e. it seems to randomly return the same file multiple times. So that's useless.
Hmm. When was the last time you tested this? I don't doubt your findings but a colleague and I were not able to reproduce this on a current OSX system, no duplicate entries could be provoked. Grüße, Sven.
On Jul 16, 2020, at 5:11 AM, Sven Hartge <sven@svenhartge.de> wrote:
On 15.07.20 17:54, Alan DeKok wrote:
Sure. The larger issue is that I tested it on OSX, and scandir() returns *duplicates*. i.e. it seems to randomly return the same file multiple times. So that's useless.
Hmm. When was the last time you tested this?
5 minutes before I replied.
I don't doubt your findings but a colleague and I were not able to reproduce this on a current OSX system, no duplicate entries could be provoked.
I sorted the entries using the alphasort() function. It was pretty definitive. One directory had one file in it, and scandir() returned an array of 3 entries, all of the same filename. For reading mods-enabled/, it randomly returned duplicates. It's pretty difficult to get scandir() wrong, or to use it wrong. If the directory has one file and scandir() returns 3, well, that's a problem. Alan DeKok.
On 16.07.20 14:29, Alan DeKok wrote:
On Jul 16, 2020, at 5:11 AM, Sven Hartge <sven@svenhartge.de> wrote:
On 15.07.20 17:54, Alan DeKok wrote:
Sure. The larger issue is that I tested it on OSX, and scandir() returns *duplicates*. i.e. it seems to randomly return the same file multiple times. So that's useless.
Hmm. When was the last time you tested this?
5 minutes before I replied.
That is ugly. And interesting.
I don't doubt your findings but a colleague and I were not able to reproduce this on a current OSX system, no duplicate entries could be provoked.
I sorted the entries using the alphasort() function. It was pretty definitive. One directory had one file in it, and scandir() returned an array of 3 entries, all of the same filename.
For reading mods-enabled/, it randomly returned duplicates.
It's pretty difficult to get scandir() wrong, or to use it wrong. If the directory has one file and scandir() returns 3, well, that's a problem.
Out if interest: can you share your test-code? I'd like to experiment with it. Grüße, Sven.
On Jul 16, 2020, at 8:33 AM, Sven Hartge <sven@svenhartge.de> wrote:
Out if interest: can you share your test-code? I'd like to experiment with it.
I already pushed a different change to "master", where I was testing this. So no, I don't have the code any more. The basic use-case was calling scandir() with NULL for the selector function, and alphasort() for the sorting function, as in your patch. It's hard for me to see how anything can go wrong... Alan DeKok.
Um 08:37 Uhr am 16.07.20 schrieb Alan DeKok:
On Jul 16, 2020, at 8:33 AM, Sven Hartge <sven@svenhartge.de> wrote:
Out if interest: can you share your test-code? I'd like to experiment with it.
The basic use-case was calling scandir() with NULL for the selector function, and alphasort() for the sorting function, as in your patch.
It's hard for me to see how anything can go wrong...
Yes, I can see that. Really interesting. Here is the simple test tool I wrote. As you can see, it also contains part of the freeradius code to remove unwanted filenames to test that *dp is really the same is the one readdir() spits out. ----------------------------8<------------------- #define _DEFAULT_SOURCE #include <dirent.h> #include <stdio.h> #include <stdlib.h> #include <ctype.h> int main(void) { struct dirent **namelist; struct dirent *dp; int n,i; n = scandir(".", &namelist, NULL, alphasort); if (n == -1) { perror("scandir"); exit(EXIT_FAILURE); } for (i=0; i<n; i++) { char const *p; dp = namelist[i]; free(namelist[i]); if (dp->d_name[0] == '.') continue; for (p = dp->d_name; *p != '\0'; p++) { if (isalpha((int)*p) || isdigit((int)*p) || (*p == '-') || (*p == '_') || (*p == '.')) continue; break; } if (*p != '\0') continue; printf("%d: %lu: %s\n", i, dp->d_ino, dp->d_name); } free(namelist); exit(EXIT_SUCCESS); } ----------------------------8<------------------- Does this also show the same issues? Grüße, Sven.
On 16 Jul 2020, at 13:56, Sven Hartge <sven@svenhartge.de> wrote:
Here is the simple test tool I wrote. As you can see, it also contains part of the freeradius code to remove unwanted filenames to test that *dp is really the same is the one readdir() spits out.
Gave this a test myself, and found it didn't report the full contents of a directory on a Mac. $ ./scandir | wc -l 57 $ ls . | wc -l 67 No apparent pattern I could see between the things it did list and the things it didn't - there's a mix of symlinks, files and directories, and scandir recorded all 3 kinds... just not every instance. Adam Bishop gpg: E75B 1F92 6407 DFDF 9F1C BF10 C993 2504 6609 D460 jisc.ac.uk Jisc is a registered charity (number 1149740) and a company limited by guarantee which is registered in England under company number. 05747339, VAT number GB 197 0632 86. Jisc’s registered office is: 4 Portwall Lane, Bristol, BS1 6NB. T 0203 697 5800. Jisc Services Limited is a wholly owned Jisc subsidiary and a company limited by guarantee which is registered in England under company number 02881024, VAT number GB 197 0632 86. The registered office is: 4 Portwall Lane, Bristol, BS1 6NB. T 0203 697 5800. Jisc Commercial Limited is a wholly owned Jisc subsidiary and a company limited by shares which is registered in England under company number 09316933, VAT number GB 197 0632 86. The registered office is: 4 Portwall Lane, Bristol, BS1 6NB. T 0203 697 5800. For more details on how Jisc handles your data see our privacy notice here: https://www.jisc.ac.uk/website/privacy-notice
On 16.07.20 15:11, Adam Bishop via Freeradius-Users wrote:
On 16 Jul 2020, at 13:56, Sven Hartge <sven@svenhartge.de> wrote:
Here is the simple test tool I wrote. As you can see, it also contains part of the freeradius code to remove unwanted filenames to test that *dp is really the same is the one readdir() spits out.
Gave this a test myself, and found it didn't report the full contents of a directory on a Mac.
$ ./scandir | wc -l 57 $ ls . | wc -l 67
Well, the test tool filters out everything containing characters other than digits, letters, '.', '-' and '_'. So if there are any entries like "lost+found" it will be skipped. Grüße, Sven.
Um 17:49 Uhr am 15.07.20 schrieb Sven Hartge:
On 15.07.20 17:14, Alan DeKok wrote:
On Jul 15, 2020, at 8:19 AM, Sven Hartge <sven@svenhartge.de> wrote:
And in the end the load order should be fixed by sorting before loading to make it stable for everyone and not break semi-randomly based on the way a filesystem presents/stores the entries in a directory.
As alway, patches are welcome.
OK, please see the attached diff.
Right, the list strips attachments. So here it is inline: diff --git a/src/main/conffile.c b/src/main/conffile.c index a8c667bfb5..a4494ef7ad 100644 --- a/src/main/conffile.c +++ b/src/main/conffile.c @@ -2476,8 +2476,9 @@ static int cf_section_read(char const *filename, int *lineno, FILE *fp, * careful! */ if (value[strlen(value) - 1] == '/') { - DIR *dir; struct dirent *dp; + struct dirent **namelist; + int n,i; struct stat stat_buf; DEBUG2("including files in directory %s", value ); @@ -2498,8 +2499,8 @@ static int cf_section_read(char const *filename, int *lineno, FILE *fp, return -1; } #endif - dir = opendir(value); - if (!dir) { + n = scandir(value, &namelist, NULL, alphasort); + if (n == -1) { ERROR("%s[%d]: Error reading directory %s: %s", filename, *lineno, value, fr_syserror(errno)); @@ -2509,9 +2510,12 @@ static int cf_section_read(char const *filename, int *lineno, FILE *fp, /* * Read the directory, ignoring "." files. */ - while ((dp = readdir(dir)) != NULL) { + for (i=0; i<n; i++) { char const *p; + dp = namelist[i]; + free(namelist[i]); + if (dp->d_name[0] == '.') continue; /* @@ -2537,11 +2541,11 @@ static int cf_section_read(char const *filename, int *lineno, FILE *fp, * configuration section. */ if (cf_file_include(this, buf2, true) < 0) { - closedir(dir); + free(namelist); return -1; } } - closedir(dir); + free(namelist); } else #endif { /* it was a normal file */
participants (4)
-
Adam Bishop -
Alan DeKok -
Matthew Newton -
Sven Hartge