Re: use-after-free in psql

[Alvaro Herrera <alvherre(at)commandprompt(dot)com>]

Neil Conway wrote:
> There's a minor bug in the ON_ERROR_ROLLBACK code in psql. In HEAD, at
> line 878 the storage pointed to by "results" is released by a PQclear(),
> but is referenced by the PQcmdStatus() calls on lines 898, 899, and 900.

I think this is the fix for HEAD.  I'll apply after testing it a bit
more.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

? common.c.fix
Index: common.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/common.c,v
retrieving revision 1.119
diff -c -p -r1.119 common.c
*** common.c	14 Jun 2006 16:49:02 -0000	1.119
--- common.c	30 Jun 2006 14:40:18 -0000
*************** SendQuery(const char *query)
*** 875,893 ****
  	if (OK)
  		OK = PrintQueryResults(results);
  
- 	PQclear(results);
- 
  	/* If we made a temporary savepoint, possibly release/rollback */
  	if (on_error_rollback_savepoint)
  	{
  		transaction_status = PQtransactionStatus(pset.db);
  
  		/* We always rollback on an error */
  		if (transaction_status == PQTRANS_INERROR)
! 			results = PQexec(pset.db, "ROLLBACK TO pg_psql_temporary_savepoint");
  		/* If they are no longer in a transaction, then do nothing */
  		else if (transaction_status != PQTRANS_INTRANS)
! 			results = NULL;
  		else
  		{
  			/*
--- 875,893 ----
  	if (OK)
  		OK = PrintQueryResults(results);
  
  	/* If we made a temporary savepoint, possibly release/rollback */
  	if (on_error_rollback_savepoint)
  	{
+ 		PGresult   *svptres;
+ 
  		transaction_status = PQtransactionStatus(pset.db);
  
  		/* We always rollback on an error */
  		if (transaction_status == PQTRANS_INERROR)
! 			svptres = PQexec(pset.db, "ROLLBACK TO pg_psql_temporary_savepoint");
  		/* If they are no longer in a transaction, then do nothing */
  		else if (transaction_status != PQTRANS_INTRANS)
! 			svptres = NULL;
  		else
  		{
  			/*
*************** SendQuery(const char *query)
*** 898,917 ****
  			if (strcmp(PQcmdStatus(results), "SAVEPOINT") == 0 ||
  				strcmp(PQcmdStatus(results), "RELEASE") == 0 ||
  				strcmp(PQcmdStatus(results), "ROLLBACK") == 0)
! 				results = NULL;
  			else
! 				results = PQexec(pset.db, "RELEASE pg_psql_temporary_savepoint");
  		}
! 		if (PQresultStatus(results) != PGRES_COMMAND_OK)
  		{
  			psql_error("%s", PQerrorMessage(pset.db));
  			PQclear(results);
  			ResetCancelConn();
  			return false;
  		}
- 		PQclear(results);
  	}
  
  	/* Possible microtiming output */
  	if (OK && pset.timing)
  		printf(_("Time: %.3f ms\n"), DIFF_MSEC(&after, &before));
--- 898,919 ----
  			if (strcmp(PQcmdStatus(results), "SAVEPOINT") == 0 ||
  				strcmp(PQcmdStatus(results), "RELEASE") == 0 ||
  				strcmp(PQcmdStatus(results), "ROLLBACK") == 0)
! 				svptres = NULL;
  			else
! 				svptres = PQexec(pset.db, "RELEASE pg_psql_temporary_savepoint");
  		}
! 		if (svptres && PQresultStatus(svptres) != PGRES_COMMAND_OK)
  		{
  			psql_error("%s", PQerrorMessage(pset.db));
  			PQclear(results);
+ 			PQclear(svptres);
  			ResetCancelConn();
  			return false;
  		}
  	}
  
+ 	PQclear(results);
+ 
  	/* Possible microtiming output */
  	if (OK && pset.timing)
  		printf(_("Time: %.3f ms\n"), DIFF_MSEC(&after, &before));