Patch: avpair.c - empty reply VALUE_PAIR (0x0) for replies with unknown vendor attributes

[Adam Lewis]

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 at 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-for-replies-with--unknown-vendor-attributes-tp28313256p28888431.html
>> 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-for-replies-with--unknown-vendor-attributes-tp28313256p29008790.html
Sent from the FreeRadius - Dev mailing list archive at Nabble.com.