Re: rlm_sql: %{sql:<long query>} - length limit
O/H Alan DeKok έγραψε:
Milan Holub wrote:
==> I've increased the value of MAX_STRING_LEN to 1024. Here is a patch:
It will break almost everything in the server.
Moved to freeradius-devel... The problem still stands that we cannot have xlat strings more than MAX_STRING_LEN. freeradius should impose a string length restriction on the xlat output, not on the input. Maybe we could dynamically allocate an input string if we see backquotes and store the xlated output in VALUE_PAIR->data?
My query works now but I'm not sure whether this change might not have some unwanted impact somewhere else since the constant is used on many places... Alan?
It's used all over the place, for things other than queries. Do NOT change it!
Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/users.html
Kostas Kalevras - Network Operations Center, NTUA http://kkalev.wordpress.com/
Kostas Kalevras wrote:
The problem still stands that we cannot have xlat strings more than MAX_STRING_LEN. freeradius should impose a string length restriction on the xlat output, not on the input. Maybe we could dynamically allocate an input string if we see backquotes and store the xlated output in VALUE_PAIR->data?
Yes. Ideally, the only part of the server that should know about MAX_STRING_LEN is the packet encoding routines. That could be done via: a) new type PW_STRING_PTR (already defined) b) vp->data holding a pointer c) auditing the code for places that write to vp->vp_strvalue I've been thinking about related issues, such as why *ever* allocate memory for strings in a VALUE_PAIR? Why not just always use a pointer? For some types, it should be possible to point the string to the packet data, which means no memcpy() (that's a non-negligible cost in the server). It may be easier to turn vp_strvalue && vp_octets into pointers. That will break all of the code, which will force an audit, to fix everything. Then, any code that writes to vp_strvalue or vp_octets would be required to call a new function: pair_replace_string(VALUE_PAIR *vp, const uint8_t *data, size_t datalen); That function would just do the right thing to get the new data into the VALUE_PAIR. It could start off in the current code with a simple memcpy/strcpy. Once all of the writes are updated to use this wrapper function, then we could update it to swap the pointers. The main difficulty is the paircopy() routine, which makes copies of the VALUE_PAIRs. Would it malloc() room for the strings it copies, or would it point to the data from the old string? Maybe simply malloc'ing it would be easier... This is one place where garbage collection would make the code MUCH easier. Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
Alan DeKok wrote:
This is one place where garbage collection would make the code MUCH easier.
Or, memory pools. Start off with a 1K or 2K pool for a REQUEST*, and allocate any REQUEST specific things from there, including VALUE_PAIRs. It would mean that the pool code would have to be in libradius, and that the pairfoo() interface would be updated to take a memory pool pointer. It would be just repetitive edits to get that done, though. I think the pool code should be pretty simple. Let's see... Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
On April 21, 2007 6:02:41 PM +0200 Alan DeKok <aland@deployingradius.com> wrote:
Alan DeKok wrote:
This is one place where garbage collection would make the code MUCH easier.
Or, memory pools. Start off with a 1K or 2K pool for a REQUEST*, and allocate any REQUEST specific things from there, including VALUE_PAIRs. It would mean that the pool code would have to be in libradius, and that the pairfoo() interface would be updated to take a memory pool pointer.
It would be just repetitive edits to get that done, though. I think the pool code should be pretty simple. Let's see...
I would try tcmalloc (Linux) or mtmalloc (Solaris) first. -frank
On Sat, Apr 21, 2007 at 11:27:47AM -0700, Frank Cusack wrote:
On April 21, 2007 6:02:41 PM +0200 Alan DeKok <aland@deployingradius.com> wrote:
Alan DeKok wrote:
This is one place where garbage collection would make the code MUCH easier.
Or, memory pools. Start off with a 1K or 2K pool for a REQUEST*, and allocate any REQUEST specific things from there, including VALUE_PAIRs. It would mean that the pool code would have to be in libradius, and that the pairfoo() interface would be updated to take a memory pool pointer.
It would be just repetitive edits to get that done, though. I think the pool code should be pretty simple. Let's see...
I would try tcmalloc (Linux) or mtmalloc (Solaris) first.
Or talloc, which lets you allocate hierarchially, so when you delete a request, all the stuff in it gets deallocated as well, without having to guess ahead of time how much space it's all gonna take. And it has destructors. http://talloc.samba.org/ -- ----------------------------------------------------------- Paul "TBBle" Hampson, B.Sc, LPI, MCSE On-hiatus Asian Studies student, ANU The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361) Paul.Hampson@Pobox.Com Of course Pacman didn't influence us as kids. If it did, we'd be running around in darkened rooms, popping pills and listening to repetitive music. -- Kristian Wilson, Nintendo, Inc, 1989 License: http://creativecommons.org/licenses/by/2.1/au/ -----------------------------------------------------------
Paul TBBle Hampson wrote:
Or talloc, which lets you allocate hierarchially, so when you delete a request, all the stuff in it gets deallocated as well, without having to guess ahead of time how much space it's all gonna take.
I've been following Samba for a while. talloc is nice, but I think it's way overkill for what we need.
And it has destructors.
Which I don't think we need. For RADIUS, all memory allocation is either global, per-module instance, or tied to a REQUEST. The memory pools are probably best used for REQUESTs. My tests indicate that malloc/free overhead for a REQUEST with few VALUE_PAIRs can be up to 10% of CPU time. So fixing that would help performance, and would likely simplify the code. What's in a REQUEST? RADIUS_PACKETs, VALUE_PAIR's, and not much else. They don't need destructors on exit, so that overhead in a memory pool can be lost. We don't need thread locks on a REQUEST, as the server design guarantees that only one thread is accessing a REQUEST at a time. I think we can have simple 2k-page memory pools with only 100 to 200 lines of code. And, that means no external dependencies, either. Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
participants (4)
-
Alan DeKok -
Frank Cusack -
Kostas Kalevras -
Paul TBBle Hampson