freeradius-client sequence file replacement

[Alex Massover]

Here's a patch removing seq file. Also there's no need in lib/lock.c anymore.


diff -ur ./BUGS /tmp/freeradius-client-1.1.6/BUGS
--- ./BUGS	2007-06-21 21:35:00.000000000 +0300
+++ /tmp/freeradius-client-1.1.6/BUGS	2010-02-01 16:54:25.203211724 +0200
@@ -3,12 +3,6 @@
 Testing can show the presense of bugs, but not their absence.
                 -- Dijkstra
 
-The get_seqnbr function in build_req.c sometimes returns the same
-number, if invoked from multiple processes at the same time. This
-occurs only if a lot of process try to read the sequence file,
-so I suspect a race condition in the locking code, but I can't
-find one. Any help is appreciated.
-
 Radlogin respectively send_server in sendserver.c does not honour a 
 ACCESS_CHALLENGE packet from the RADIUS server. 
 
diff -ur ./include/freeradius-client.h /tmp/freeradius-client-1.1.6/include/freeradius-client.h
--- ./include/freeradius-client.h	2008-02-11 08:54:23.000000000 +0200
+++ /tmp/freeradius-client-1.1.6/include/freeradius-client.h	2010-02-01 16:47:17.084878041 +0200
@@ -426,7 +426,7 @@
 /*	buildreq.c		*/
 
 void rc_buildreq(rc_handle *, SEND_DATA *, int, char *, unsigned short, char *, int, int);
-unsigned char rc_get_seqnbr(rc_handle *);
+unsigned char rc_get_id();
 int rc_auth(rc_handle *, uint32_t, VALUE_PAIR *, VALUE_PAIR **, char *);
 int rc_auth_proxy(rc_handle *, VALUE_PAIR *, VALUE_PAIR **, char *);
 int rc_acct(rc_handle *, uint32_t, VALUE_PAIR *);
diff -ur ./lib/buildreq.c /tmp/freeradius-client-1.1.6/lib/buildreq.c
--- ./lib/buildreq.c	2008-03-05 18:35:20.000000000 +0200
+++ /tmp/freeradius-client-1.1.6/lib/buildreq.c	2010-02-01 17:29:46.987179691 +0200
@@ -13,7 +13,7 @@
 #include <includes.h>
 #include <freeradius-client.h>
 
-unsigned char rc_get_seqnbr(rc_handle *);
+unsigned char rc_get_id();
 
 /*
  * Function: rc_buildreq
@@ -29,87 +29,26 @@
 	data->server = server;
 	data->secret = secret;
 	data->svc_port = port;
-	data->seq_nbr = rc_get_seqnbr(rh);
+	data->seq_nbr = rc_get_id();
 	data->timeout = timeout;
 	data->retries = retries;
 	data->code = code;
 }
 
 /*
- * Function: rc_guess_seqnbr
+ * Function: rc_get_id
  *
- * Purpose: return a random sequence number
+ * Purpose: generate random id
  *
  */
 
-static unsigned char rc_guess_seqnbr(void)
+unsigned char rc_get_id()
 {
 	srandom((unsigned int)(time(NULL)+getpid()));
 	return (unsigned char)(random() & UCHAR_MAX);
 }
 
 /*
- * Function: rc_get_seqnbr
- *
- * Purpose: generate a sequence number
- *
- */
-
-unsigned char rc_get_seqnbr(rc_handle *rh)
-{
-	FILE *sf;
-	int tries = 1;
-	int seq_nbr;
-	char *seqfile = rc_conf_str(rh, "seqfile");
-
-	if ((sf = fopen(seqfile, "a+")) == NULL)
-	{
-		rc_log(LOG_ERR,"rc_get_seqnbr: couldn't open sequence file %s: %s", seqfile, strerror(errno));
-		/* well, so guess a sequence number */
-		return rc_guess_seqnbr();
-	}
-
-	while (do_lock_exclusive(sf)!= 0)
-	{
-		if (errno != EWOULDBLOCK) {
-			rc_log(LOG_ERR, "rc_get_seqnbr: flock failure: %s: %s", seqfile, strerror(errno));
-			fclose(sf);
-			return rc_guess_seqnbr();
-		}
-		tries++;
-		if (tries <= 10)
-			rc_mdelay(500);
-		else
-			break;
-	}
-
-	if (tries > 10) {
-		rc_log(LOG_ERR,"rc_get_seqnbr: couldn't get lock after %d tries: %s", tries-1, seqfile);
- 		fclose(sf);
-		return rc_guess_seqnbr();
-	}
-
-	rewind(sf);
-	if (fscanf(sf, "%d", &seq_nbr) != 1) {
-		rc_log(LOG_ERR,"rc_get_seqnbr: fscanf failure: %s", seqfile);
-		seq_nbr = rc_guess_seqnbr();
-	}
-
-	rewind(sf);
-	ftruncate(fileno(sf),0);
-	fprintf(sf,"%d\n", (seq_nbr+1) & UCHAR_MAX);
-
-	fflush(sf); /* fflush because a process may read it between the do_unlock and fclose */
-
-	if (do_unlock(sf) != 0)
-		rc_log(LOG_ERR, "rc_get_seqnbr: couldn't release lock on %s: %s", seqfile, strerror(errno));
-
-	fclose(sf);
-
-	return (unsigned char)seq_nbr;
-}
-
-/*
  * Function: rc_aaa
  *
  * Purpose: Builds an authentication/accounting request for port id client_port





> -----Original Message-----
> From: freeradius-devel-bounces+alex=jajah.com at lists.freeradius.org
> [mailto:freeradius-devel-bounces+alex=jajah.com at lists.freeradius.org]
> On Behalf Of Alan DeKok
> Sent: יום ב 01 פברואר 2010 15:20
> To: FreeRadius developers mailing list
> Subject: Re: freeradius-client sequence file replacement
> 
> Alex Massover wrote:
> > That requires a redesign of the way freeradius-client works, because
> currently it opens new socket for every request.
> 
>   Then set the ID field (seq_nbr) to a random number.
> 
> > Now seq file really limits performance, so for short term I can just
> drop rc_get_seqnbr() in favor of random number (random code already
> exists there). Anyway it doesn't matter what id will be there because
> new socket will be used.
> 
>   Yes.  Set it to a random number.  Even rand() here is good enough.
> 
> > For a long term maybe it worth implementing what you described below,
> or maybe it's good enough to stay with new socket?
> 
>   For high performance systems, having multiple packets on one socket
> would be best.
> 
>   Alan DeKok.
> -
> List info/subscribe/unsubscribe? See
> http://www.freeradius.org/list/devel.html
> 
> This mail was received via Mail-SeCure System.
> 


This mail was sent via Mail-SeCure System.