Fix Bug 413 sqlippool_query1 freeing results too early

Cleanup a bunch of other rubbish including a memory leak in sql_postgresql.c

diff --git a/src/modules/rlm_sql/drivers/rlm_sql_postgresql/sql_postgresql.c b/src/modules/rlm_sql/drivers/rlm_sql_postgresql/sql_postgresql.c
index 8f99560..4c49e8b 100644 (file)
--- a/src/modules/rlm_sql/drivers/rlm_sql_postgresql/sql_postgresql.c
+++ b/src/modules/rlm_sql/drivers/rlm_sql_postgresql/sql_postgresql.c
@@ -63,16 +63,19 @@ typedef struct rlm_sql_postgres_sock {
 } rlm_sql_postgres_sock;
 
 /* Prototypes */
+
+/*
 static int sql_num_fields(SQLSOCK * sqlsocket, SQL_CONFIG *config);
+*/
 
 /* Internal function. Return true if the postgresql status value
  * indicates successful completion of the query. Return false otherwise
- */
 static int
 status_is_ok(ExecStatusType status)
 {
        return status == PGRES_COMMAND_OK || status == PGRES_TUPLES_OK;
 }
+*/
 
 
 /* Internal function. Return the number of affected rows of the result
@@ -102,18 +105,26 @@ free_result_row(rlm_sql_postgres_sock * pg_sock)
 }
 
 
-/***********************************************************************
-* Check to see if we could try to reconnect after a query-server error
-* we are checking only first 2 characters according to the error code
-* class..time consuming...
-************************************************************************/
-static int sql_check_error(char *errcode)
+/*************************************************************************
+*      Function: check_fatal_error
+*      
+*      Purpose:  Check error type and behave accordingly
+*
+*************************************************************************/
+
+static int check_fatal_error (char *errorcode)
 {
        int x = 0;
 
+       /*      
+       Check the error code to see if we should reconnect or not
+       Error Code table taken from
+       http://www.postgresql.org/docs/8.1/interactive/errcodes-appendix.html
+       */
+
        while(errorcodes[x].errorcode != NULL){
-               if (strcmp(errorcodes[x].errorcode, errcode) == 0){
-                       radlog(L_DBG, "rlm_sql_postgresql: Postgresql Fatal Error: [%s] Occurred!!", errorcodes[x].meaning);
+               if (strcmp(errorcodes[x].errorcode, errorcode) == 0){
+                       radlog(L_DBG, "rlm_sql_postgresql: Postgresql Fatal Error: [%s: %s] Occurred!!", errorcode, errorcodes[x].meaning);
                        if (errorcodes[x].shouldreconnect == 1)
                                return SQL_DOWN;
                        else
@@ -121,29 +132,10 @@ static int sql_check_error(char *errcode)
                }       
                x++;
        }
-       
-       return SQL_DOWN;        
-}
 
-/*************************************************************************
-*      Function: check_fatal_error
-*      
-*      Purpose:  check and behave according to the error type we get 
-*                        from query.   
-*
-*      Note:     This will not cause reconnects because of errors other than
-*                        connection errors
-*                        Tuyan Ozipek  <tuyan@suntel.com.tr>           
-*************************************************************************/
-
-static int check_fatal_error (char *errorcode)
-{
-       /*      We have the error code in chars..
-               please have a look at table at the web page
-               http://www.postgresql.org/docs/8.1/interactive/errcodes-appendix.html
-       */
        radlog(L_DBG, "rlm_sql_postgresql: Postgresql Fatal Error: [%s] Occurred!!", errorcode);        
-       return sql_check_error(errorcode);
+       /*      We don't seem to have a matching error class/code */
+       return -1;
 }
 
 
@@ -224,17 +216,22 @@ static int sql_query(SQLSOCK * sqlsocket, SQL_CONFIG *config, char *querystr) {
        }
 
        pg_sock->result = PQexec(pg_sock->conn, querystr);
-               /* Returns a result pointer or possibly a NULL pointer.
-                * A non-NULL pointer will generally be returned except in
+               /*
+                * Returns a PGresult pointer or possibly a null pointer.
+                * A non-null pointer will generally be returned except in
                 * out-of-memory conditions or serious errors such as inability
-                * to send the command to the backend. If a NULL is returned,
-                *  it should be treated like a PGRES_FATAL_ERROR result.
-                * Use PQerrorMessage to get more information about the error.
+                * to send the command to the server. If a null pointer is
+                * returned, it should be treated like a PGRES_FATAL_ERROR
+                * result.
                 */
        if (!pg_sock->result)
        {
                radlog(L_ERR, "rlm_sql_postgresql: PostgreSQL Query failed Error: %s",
                                PQerrorMessage(pg_sock->conn));
+               /* As this error COULD be a connection error OR an out-of-memory
+                * condition return value WILL be wrong SOME of the time regardless!
+                * Pick your poison....
+                */
                return  SQL_DOWN;
        } else {
                ExecStatusType status = PQresultStatus(pg_sock->result);
@@ -290,6 +287,16 @@ static int sql_query(SQLSOCK * sqlsocket, SQL_CONFIG *config, char *querystr) {
 
                        break;
 
+                       default:
+                               /* FIXME: An unhandled error occurred.*/
+
+                               /* PGRES_EMPTY_QUERY PGRES_COPY_OUT PGRES_COPY_IN */
+
+                               return -1;
+
+                       break;
+
+
                }       
        
                /*
@@ -343,8 +350,8 @@ static int sql_destroy_socket(SQLSOCK *sqlsocket, UNUSED SQL_CONFIG *config)
  *     Function: sql_fetch_row
  *
  *     Purpose: database specific fetch_row. Returns a SQL_ROW struct
- *               with all the data for the query in 'sqlsocket->row'. Returns
- *              0 on success, -1 on failure, SQL_DOWN if 'database is down'.
+ *     with all the data for the query in 'sqlsocket->row'. Returns
+ *     0 on success, -1 on failure, SQL_DOWN if 'database is down'.
  *
  *************************************************************************/
 static int sql_fetch_row(SQLSOCK * sqlsocket, UNUSED SQL_CONFIG *config) {
@@ -397,12 +404,8 @@ static int sql_free_result(SQLSOCK * sqlsocket, UNUSED SQL_CONFIG *config) {
                PQclear(pg_sock->result);
                pg_sock->result = NULL;
        }
-#if 0
-       /*
-        *  Commented out because it appears to free memory too early.
-        */
+
        free_result_row(pg_sock);
-#endif
 
        return 0;
 }

diff --git a/src/modules/rlm_sqlippool/rlm_sqlippool.c b/src/modules/rlm_sqlippool/rlm_sqlippool.c
index f7ed9cb..dfaaa71 100644 (file)
--- a/src/modules/rlm_sqlippool/rlm_sqlippool.c
+++ b/src/modules/rlm_sqlippool/rlm_sqlippool.c
@@ -313,7 +313,7 @@ static int sqlippool_query1(char * out, int outlen, const char * fmt, SQLSOCK *
        char expansion[MAX_STRING_LEN * 4];
        char query[MAX_STRING_LEN * 4];
        SQL_ROW row;
-       int r;
+       int rlen, retval = 0;
 
        sqlippool_expand(expansion, sizeof(expansion), fmt, instance, param, param_len);
 
@@ -340,39 +340,21 @@ static int sqlippool_query1(char * out, int outlen, const char * fmt, SQLSOCK *
                return 0;
        }
 
-       r = rlm_sql_fetch_row(sqlsocket, data->sql_inst);
-       (data->sql_inst->module->sql_finish_select_query)(sqlsocket, data->sql_inst->config);
-
-       if (r) {
-               DEBUG("sqlippool_query1: SQL query did not succeed");
-               out[0] = '\0';
-               return 0;
-       }
+       out[0] = '\0';
 
-       row = sqlsocket->row;
-       if (row == NULL) {
-               DEBUG("sqlippool_query1: SQL query did not return any results");
-               out[0] = '\0';
-               return 0;
-       }
+       if (!rlm_sql_fetch_row(sqlsocket, data->sql_inst)) {
+               if (sqlsocket->row) {
+                       if (sqlsocket->row[0]) {
+                               if ((rlen = strlen(sqlsocket->row[0])) < outlen) {
+                                       strcpy(out, sqlsocket->row[0]);
+                                       retval = rlen;
+                               } else DEBUG("sqlippool_query1: insufficient string space");
+                       } else DEBUG("sqlippool_query1: row[0] returned NULL");
+               } else DEBUG("sqlippool_query1: SQL query did not return any results");
+       } else DEBUG("sqlippool_query1: SQL query did not succeed");
 
-       if (row[0] == NULL){
-               DEBUG("sqlippool_query1: row[0] returned NULL");
-               out[0] = '\0';
-               return 0;
-       }
-
-       r = strlen(row[0]);
-       if (r >= outlen){
-               DEBUG("sqlippool_query1: insufficient string space");
-               out[0] = '\0';
-               return 0;
-       }
-
-       strncpy(out, row[0], r);
-       out[r] = '\0';
-
-       return r;
+       (data->sql_inst->module->sql_finish_select_query)(sqlsocket, data->sql_inst->config);
+       return retval;
 }
 
 static int sqlippool_initialize_sql(void * instance)
@@ -601,9 +583,8 @@ static int sqlippool_postauth(void *instance, REQUEST * request)
        }
 
        
-       /*  Reminder to self </tuyan>
-        *  please make it work with the ipv6 address'
-        *  before the freeradius v2 release 
+       /*
+        *  FIXME: Make it work with the ipv6 addresses
         */
        if ((ip_hton(allocation, AF_INET, &ipaddr) < 0) ||
            ((ip_allocation = ipaddr.ipaddr.ip4addr.s_addr) == INADDR_NONE))