reply attributes not always sent (radius_paircreate() problem seen in rlm_ippool)
John Dennis <jdennis@redhat.com> wrote:
-- John
------------------------------------------------------------------------
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
I reported that a while back I thought Alan fixed it? Did it go on master and not 3.x branch? -- Sent from my phone with, please excuse brevity and typos
Phil Mayers <p.mayers@imperial.ac.uk> wrote:
John Dennis <jdennis@redhat.com> wrote:
-- John
------------------------------------------------------------------------
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
I reported that a while back I thought Alan fixed it? Did it go on master and not 3.x branch? -- Sent from my phone with, please excuse brevity and typos
Yeah see http://freeradius.1045715.n5.nabble.com/sqlippool-in-3-0-td5722879.html -- Sent from my phone with, please excuse brevity and typos
On 01/18/2014 11:52 AM, Phil Mayers wrote:
Phil Mayers <p.mayers@imperial.ac.uk> wrote:
John Dennis <jdennis@redhat.com> wrote:
I reported that a while back I thought Alan fixed it? Did it go on master and not 3.x branch?
Yeah see
http://freeradius.1045715.n5.nabble.com/sqlippool-in-3-0-td5722879.html
Well apparently not because I was testing with 3.0.1 which only a couple of days old. -- John
On 01/18/2014 03:58 PM, Alan DeKok wrote:
John Dennis wrote:
Well apparently not because I was testing with 3.0.1 which only a couple of days old.
I've added one-line patch: commit 82d3d851e
Thank you Alan. I wasn't sure the right place to add the initialization, pairalloc makes sense. -- John
Hi,
I've added one-line patch: commit 82d3d851e
So, does this affect only ippool, or reply attributes all over the place? I'm asking because I'm wondering if backporting is important for my install. Stefan
The patch affects *all* attributes made via paircreate(). I'll take a pass through the modules to remove code which sets the vp length manually. It should not be necessary for ipv4 attributes, etc. Sent from my iPhone On 2014-01-19, at 1:55 PM, Stefan Winter <stefan.winter@restena.lu> wrote:
Hi,
I've added one-line patch: commit 82d3d851e
So, does this affect only ippool, or reply attributes all over the place? I'm asking because I'm wondering if backporting is important for my install.
Stefan - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
On 01/19/2014 02:21 PM, Alan DeKok wrote:
The patch affects *all* attributes made via paircreate().
I'll take a pass through the modules to remove code which sets the vp length manually. It should not be necessary for ipv4 attributes, etc.
commit 0b48c180 for rlm_sqlipool is one that should be removed, it would break if and when ipv6 is supported. -- John
On 01/19/2014 03:09 PM, John Dennis wrote:
On 01/19/2014 02:21 PM, Alan DeKok wrote:
The patch affects *all* attributes made via paircreate().
I'll take a pass through the modules to remove code which sets the vp length manually. It should not be necessary for ipv4 attributes, etc.
commit 0b48c180 for rlm_sqlipool is one that should be removed, it would break if and when ipv6 is supported.
While you're at it could you also fix the warning messages in rad_encode() (radius.c around line 1850) so they're emitted. Maybe they should be DEBUG statements instead of fr_strerror_printf() or maybe fix fr_strerror_printf(), either way it should show up when running in debug -X mode. -- John
On 19 Jan 2014, at 22:06, John Dennis <jdennis@redhat.com> wrote:
On 01/19/2014 03:09 PM, John Dennis wrote:
On 01/19/2014 02:21 PM, Alan DeKok wrote:
The patch affects *all* attributes made via paircreate().
I'll take a pass through the modules to remove code which sets the vp length manually. It should not be necessary for ipv4 attributes, etc.
commit 0b48c180 for rlm_sqlipool is one that should be removed, it would break if and when ipv6 is supported.
While you're at it could you also fix the warning messages in rad_encode() (radius.c around line 1850) so they're emitted. Maybe they should be DEBUG statements instead of fr_strerror_printf() or maybe fix fr_strerror_printf()
What evidence do you have that fr_strerror_printf() isn't working as intended?
, either way it should show up when running in debug -X mode.
Everything in src/lib belongs to libfreeradius-radius which is a separate body of code from the rest of FreeRADIUS. It's up to the application (in this case radiusd) to call fr_strerror() or fr_perror() after getting an error code from one of the library functions. There's no obvious place where that doesn't happen for rad_encode() and the library functions which call it. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2
John Dennis wrote:
While you're at it could you also fix the warning messages in rad_encode() (radius.c around line 1850) so they're emitted. Maybe they should be DEBUG statements instead of fr_strerror_printf() or maybe fix fr_strerror_printf(), either way it should show up when running in debug -X mode.
It's not always an error to miss encoding attributes. The call to rad_encode() succeeds, so there's no reason for the radiusd binary to print out an error. The issue is that rad_encode() calls rad_vp2attr(). So maybe rad_encode() should print the error string, via a DEBUG statement. IMHO what should really happen is that we should have a "sanity check" module. It should check outgoing packets for standards compliance (mostly), and a number of "bad" situations. It should also check incoming packets, too. Alan DeKok.
On 20 Jan 2014, at 15:32, Alan DeKok <aland@deployingradius.com> wrote:
John Dennis wrote:
While you're at it could you also fix the warning messages in rad_encode() (radius.c around line 1850) so they're emitted. Maybe they should be DEBUG statements instead of fr_strerror_printf() or maybe fix fr_strerror_printf(), either way it should show up when running in debug -X mode.
It's not always an error to miss encoding attributes. The call to rad_encode() succeeds, so there's no reason for the radiusd binary to print out an error. The issue is that rad_encode() calls rad_vp2attr().
and rad_vp2attr() passes back error codes from the other attribute encoders, and those encoders log errors using fr_strerror_printf(). rad_encode() doesn't call fr_strerror_printf() itself on rad_vp2attr() error, so it seems some place in the code isn't checking the return value of rad_encode().
So maybe rad_encode() should print the error string, via a DEBUG statement.
It's a library function. It shouldn't even have the DEBUG() macro available. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2
On 20 Jan 2014, at 17:24, Arran Cudbard-Bell <a.cudbardb@freeradius.org> wrote:
On 20 Jan 2014, at 15:32, Alan DeKok <aland@deployingradius.com> wrote:
John Dennis wrote:
While you're at it could you also fix the warning messages in rad_encode() (radius.c around line 1850) so they're emitted. Maybe they should be DEBUG statements instead of fr_strerror_printf() or maybe fix fr_strerror_printf(), either way it should show up when running in debug -X mode.
It's not always an error to miss encoding attributes. The call to rad_encode() succeeds, so there's no reason for the radiusd binary to print out an error. The issue is that rad_encode() calls rad_vp2attr().
and rad_vp2attr() passes back error codes from the other attribute encoders, and those encoders log errors using fr_strerror_printf(). rad_encode() doesn't call fr_strerror_printf() itself on rad_vp2attr() error, so it seems some place in the code isn't checking the return value of rad_encode().
So maybe rad_encode() should print the error string, via a DEBUG statement.
It's a library function. It shouldn't even have the DEBUG() macro available.
OK. Fixed the fr_strerror_printf() issue. See https://github.com/FreeRADIUS/freeradius-server/commit/d253c7306dc5170972dc7... Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2
John Dennis <jdennis@redhat.com> wrote:
On 01/18/2014 11:52 AM, Phil Mayers wrote:
Phil Mayers <p.mayers@imperial.ac.uk> wrote:
John Dennis <jdennis@redhat.com> wrote:
I reported that a while back I thought Alan fixed it? Did it go on master and not 3.x branch?
Yeah see
http://freeradius.1045715.n5.nabble.com/sqlippool-in-3-0-td5722879.html
Well apparently not because I was testing with 3.0.1 which only a couple of days old.
-- John
Quite right misread your email mine was with sqlippool. Guess there has been some general change w.r.t. vp_length initialisation as per your suggestion. Some possible catch-bugs are ref'ed in the thread IIRC. (Forgive terse response I am currently on snowboarding trip) -- Sent from my phone with, please excuse brevity and typos
participants (5)
-
Alan DeKok -
Arran Cudbard-Bell -
John Dennis -
Phil Mayers -
Stefan Winter