[Asterisk-code-review] cdr pgsql: Fix buffer overflow calling libpq (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/5365 )

Change subject: cdr_pgsql: Fix buffer overflow calling libpq
......................................................................


Patch Set 2: Code-Review-1

(2 comments)

https://gerrit.asterisk.org/#/c/5365/2/cdr/cdr_pgsql.c
File cdr/cdr_pgsql.c:

PS2, Line 388: 							if (!tmpbuf) {
             : 								goto ast_log_cleanup;
Need to
AST_RWLIST_UNLOCK(&psql_columns);
goto ast_log_cleanup;

Ooops.  This was missed in the cel_pgsql.c patch too.


PS2, Line 436: 		if (PQresultStatus(result) != PGRES_COMMAND_OK) {
             : 			pgerror = PQresultErrorMessage(result);
             : 			ast_log(LOG_ERROR, "Failed to insert call detail record into database!\n");
             : 			ast_log(LOG_ERROR, "Reason: %s\n", pgerror);
             : 			ast_log(LOG_ERROR, "Connection may have been lost... attempting to reconnect.\n");
             : 			PQclear(result);
             : 			PQreset(conn);
             : 			if (PQstatus(conn) == CONNECTION_OK) {
             : 				ast_log(LOG_ERROR, "Connection reestablished.\n");
             : 				connected = 1;
             : 				connect_time = time(NULL);
             : 				records = 0;
             : 				result = PQexec(conn, ast_str_buffer(sql));
             : 				if (PQresultStatus(result) != PGRES_COMMAND_OK) {
             : 					pgerror = PQresultErrorMessage(result);
             : 					ast_log(LOG_ERROR, "HARD ERROR!  Attempted reconnection failed.  DROPPING CALL RECORD!\n");
             : 					ast_log(LOG_ERROR, "Reason: %s\n", pgerror);
             : 				} else {
             : 					/* Second try worked out ok */
             : 					totalrecords++;
             : 					records++;
             : 					res = 0;
             : 				}
             : 				PQclear(result);
             : 			}
             : 		} else {
             : 			PQclear(result);
             : 			totalrecords++;
             : 			records++;
             : 			res = 0;
             : 		}
It is fine the way you have changed it.

However, PQclear(result) could have been placed this way and a couple of them wouldn't have needed to be moved and you wouldn't need as many:

result = PQexec()
if (PQresultStatus()...) {
   if (PQstatus()...) {
      PQclear(result)
      result = PQexec()
      if (PQresultStatus()...) {
      } else {
      }
   } else {
   }
} else {
}
PQclear(result)


-- 
To view, visit https://gerrit.asterisk.org/5365
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaacfa1f1de7cb1e9414d121850d2d8c2888f3f48
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes