Hello Alan, No, but it seems to be a real issue. I'm not sure why recursing over

the attributes is any easier than a simple loop.

This piece of code is so confusing and messy - won't be a bad idea to rework the whole section with recursions after recursions after recursions... :-) Rgds, Stefan

Stefan Karss wrote:

I was about to rewrite the messy recursion in recursion loop to a simple loop - and at the moment I have working code for all of my test cases.

That's good.

What I read from the RFC is that it's possible to have structured attributes in the Vendor-Specific AVPs.

The Vendor-Specific attribute can have multiple VSAs encoded in it. This isn't that common, however.

If I interpret this part of code in the current version correctly those attributes won't work in freeradius-client - is that right? Or do I miss anything?

I don't think it's working right now.

If it's not there at the moment, should it be in a new version or not?

Yes. Alan DeKok.

Hi Stefan, I am using your patch [from Apr 21] to address the problem of the unknown vendor. I have also used the same approach for standard attributes we want to ignore [Microsoft Server 2008 defaults to sending the "class" attribute which we don't have support for]. The additional problem I was talking about was to do with the effect of trying to ignore the last attribute in the list. In this case, pair is returned but it still set to null. My solution to this - not necessarily the way forward, but at least a workaround - was to rename rc_avpair_gen() to old_rc_avpair_gen() and then wrap it a new version of rc_avpair_gen() which adds a dummy pair and then removes it when old_rc_avpair_gen() returns. For what it's worth, the code for (the new) rc_avpair_gen() is as follows: /* * * Function: rc_avpair_gen * * * there's a bug in rc_avpair_gen() [now renamed old_rc_avpair_gen()) which doesn't handle an attribute * correctly if it is the last attribute in the list and the vendor is unknown. The attribute is meant * to be ignored but what happens is that all other attributes also get ignored. * * The solution is to add a dummy pair which is then treated as the (already handled) last attribute. * Use this function as a wrapper for the old function. * * Returns: value_pair list or NULL on failure */ VALUE_PAIR * rc_avpair_gen(const rc_handle *rh, VALUE_PAIR *pair, unsigned char *ptr, int length, int vendorpec) { VALUE_PAIR *dummy_pair_p; VALUE_PAIR *temp_pair_p; VALUE_PAIR *pair_p; if (pair != NULL) { rc_log(LOG_ERR, "rc_avpair_gen (new): unexpected non-null pair input"); return NULL; } dummy_pair_p = malloc(sizeof(*dummy_pair_p)); if (dummy_pair_p == NULL) { rc_log(LOG_CRIT, "rc_avpair_gen (new): out of memory"); return NULL; } memset(dummy_pair_p, '\0', sizeof(*dummy_pair_p)); dummy_pair_p->next = NULL; strcpy(dummy_pair_p->name, "DUMMY"); dummy_pair_p->attribute = 0; dummy_pair_p->type = PW_TYPE_INTEGER; dummy_pair_p->vp_integer = 0; pair_p = old_rc_avpair_gen(rh, dummy_pair_p, ptr, length, vendorpec); if (pair_p != NULL) { /* remove the dummy pair_p from the end of the list */ temp_pair_p = pair_p; while ((temp_pair_p->next != dummy_pair_p) && (temp_pair_p->next != NULL)) { temp_pair_p = temp_pair_p->next; } if (temp_pair_p == NULL) { rc_log(LOG_ERR, "rc_avpair_gen (new): pair not found"); /* this should never happen - something is wrong with the coding logic. clean up as best we can */ while (pair_p != NULL) { temp_pair_p = pair_p->next; free(pair_p); pair_p = temp_pair_p; } } else { temp_pair_p->next = NULL; } free(dummy_pair_p); } return pair_p; } Stefan Karss wrote:

Hello Adam,

I guess my description was a bit confusing - "first recursion" means the first recursion instance in that bit of messy code ;-). So wherever an unknown vendor happens the reply pointer is NULL.

Try my patch and see if it helps. If I remember correctly someone on the list wanted to submit another patch that translates unknown attributes/vendors into a special syntax - but I'm not sure if it has already been submitted.

Rgds, Stefan

On Tue, Jun 15, 2010 at 10:12 AM, Adam Lewis <VNQM87@motorola.com> wrote:

...

I also seem to be having a problem with attributes that should be ignored but otherwise have no effect on the other attributes in a reply.

If an attribute from an unknown vendor happens to be the very last attribute in the list, rc_avpair_gen() ignores it by returning pair immediately. The only problem being pair is still set to the entry value of NULL and this ripples up through all the recursive calls, which means all the other attributes get ignored as well. Has anybody else seen this?

Adam.

Stefan Karss wrote:

...

Hello again,

when the RADIUS servers sends a reply packet with an attribute for an unknown vendor freeradius-client just skips over this when filling response VALUE_PAIR list. Unfortunately it doesn't for unknown attributes from known vendors - in that case rc_avpair_gen returns NULL in the first

recursion

...

which results in a NULL pointer for reply VALUE_PAIR.

The attached patch just skips over unknown attributes for known vendors just like the lib skips over attributes from unknown vendors.

Rgds, Stefan

- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

-- View this message in context: http://old.nabble.com/Patch%3A-avpair.c---empty-reply-VALUE_PAIR-%280x0%29-f... Sent from the FreeRadius - Dev mailing list archive at Nabble.com.

- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

-- View this message in context: http://old.nabble.com/Patch%3A-avpair.c---empty-reply-VALUE_PAIR-%280x0%29-f... Sent from the FreeRadius - Dev mailing list archive at Nabble.com.

How to debug the unknown attributes? Put some prints in the code or have clear way to do this?

Adam Lewis wrote:

...

My solution to this - not necessarily the way forward, but at least a workaround - was to rename rc_avpair_gen() to old_rc_avpair_gen() and then wrap it a new version of rc_avpair_gen() which adds a dummy pair and then removes it when old_rc_avpair_gen() returns. For what it's worth, the code for (the new) rc_avpair_gen() is as follows:

The real fix is to do what the server does. Mark unknown attributes as "unknown", and pass them straight through the system.

Alan DeKok.

Just a warning to anyone using the client on an embedded platform. The local variable "buffer" in the function rc_avpair_gen() uses 507 bytes, so if the function has many attributes to process, 507 bytes will be added to the stack every time the function recurses. Perhaps another reason why this function should be re-written. Regards, Adam. Alan DeKok-2 wrote:

Toledo, Luis Carlos wrote:

...

How to debug the unknown attributes? Put some prints in the code or have clear way to do this?

The code should print out which attributes it receives. If it doesn't, supply a patch to fix it.

Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

-- View this message in context: http://old.nabble.com/Patch%3A-avpair.c---empty-reply-VALUE_PAIR-%280x0%29-f... Sent from the FreeRadius - Dev mailing list archive at Nabble.com.