Slightly refactored the BLR parser routines to avoid crazy error reporting and protect against NULL pointer dereference. The error handling still sucks, but it was the case before me ;-)

src/remote/client/interface.cpp

@@ -1203,9 +1203,8 @@ Firebird::IRequest* Attachment::compileRequest(IStatus* status,
 
 		RMessage* message = PARSE_messages(blr, blr_length);
 		USHORT max_msg = 0;
-		for (next = message; next; next = next->msg_next) {
+		for (next = message; next; next = next->msg_next)
 			max_msg = MAX(max_msg, next->msg_number);
-		}
 
 		// Allocate request block
 		Rrq* request = new Rrq(max_msg + 1);
@@ -1737,14 +1736,7 @@ Firebird::ITransaction* Statement::execute(IStatus* status, Firebird::ITransacti
 		// Parse the blr describing the message, if there is any.
 
 		if (in_blr_length)
-		{
+			statement->rsr_bind_format = PARSE_msg_format(in_blr, in_blr_length);
-			RMessage* message = PARSE_messages(in_blr, in_blr_length);
-			if (message != (RMessage*) - 1)
-			{
-				statement->rsr_bind_format = (rem_fmt*) message->msg_address;
-				delete message;
-			}
-		}
 
 		// Parse the blr describing the output message.  This is not the fetch
 		// message!  That comes later.
@@ -1754,12 +1746,7 @@ Firebird::ITransaction* Statement::execute(IStatus* status, Firebird::ITransacti
 			if (!port->port_statement)
 				port->port_statement = new Rsr;
 
-			RMessage* message = PARSE_messages(out_blr, out_blr_length);
+			port->port_statement->rsr_select_format = PARSE_msg_format(out_blr, out_blr_length);
-			if (message != (RMessage*) - 1)
-			{
-				port->port_statement->rsr_select_format = (rem_fmt*) message->msg_address;
-				delete message;
-			}
 
 			if (!port->port_statement->rsr_buffer)
 			{
@@ -1932,14 +1919,7 @@ ResultSet* Statement::openCursor(IStatus* status, Firebird::ITransaction* apiTra
 		// Parse the blr describing the message, if there is any.
 
 		if (in_blr_length)
-		{
+			statement->rsr_bind_format = PARSE_msg_format(in_blr, in_blr_length);
-			RMessage* message = PARSE_messages(in_blr, in_blr_length);
-			if (message != (RMessage*) -1)
-			{
-				statement->rsr_bind_format = (rem_fmt*) message->msg_address;
-				delete message;
-			}
-		}
 
 		RMessage* message = NULL;
 		if (!statement->rsr_buffer)
@@ -2113,23 +2093,10 @@ ITransaction* Attachment::execute(IStatus* status, ITransaction* apiTra,
 		if (in_msg_length || out_msg_length)
 		{
 			if (in_blr_length)
-			{
+				statement->rsr_bind_format = PARSE_msg_format(in_blr, in_blr_length);
-				RMessage* message = PARSE_messages(in_blr, in_blr_length);
+
-				if (message != (RMessage*) - 1)
-				{
-					statement->rsr_bind_format = (rem_fmt*) message->msg_address;
-					delete message;
-				}
-			}
 			if (out_blr_length)
-			{
+				statement->rsr_select_format = PARSE_msg_format(out_blr, out_blr_length);
-				RMessage* message = PARSE_messages(out_blr, out_blr_length);
-				if (message != (RMessage*) - 1)
-				{
-					statement->rsr_select_format = (rem_fmt*) message->msg_address;
-					delete message;
-				}
-			}
 		}
 
 		RMessage* message = 0;
@@ -2830,14 +2797,9 @@ int ResultSet::fetchNext(IStatus* status, void* buffer)
 			{
 				delete statement->rsr_user_select_format;
 			}
-			RMessage* message = PARSE_messages(blr, blr_length);
+
-			if (message != (RMessage*) - 1)
+			statement->rsr_user_select_format = PARSE_msg_format(blr, blr_length);
-			{
+
-				statement->rsr_user_select_format = (rem_fmt*) message->msg_address;
-				delete message;
-			}
-			else
-				statement->rsr_user_select_format = NULL;
 			if (statement->rsr_flags.test(Rsr::FETCHED))
 				blr_length = 0;
 			else
@@ -5034,8 +4996,6 @@ void Attachment::transactRequest(IStatus* status, ITransaction* apiTra,
 		procedure->rpr_out_format = NULL;
 
 		RMessage* message = PARSE_messages(blr, blr_length);
-		if (message != (RMessage*) - 1)
-		{
 		while (message)
 		{
 			switch (message->msg_number)
@@ -5061,9 +5021,6 @@ void Attachment::transactRequest(IStatus* status, ITransaction* apiTra,
 				break;
 			}
 		}
-		}
-		//else
-		//	error
 
 
 		PACKET* packet = &rdb->rdb_packet;

src/remote/parse_proto.h

@@ -25,5 +25,6 @@
 #define REMOTE_PARSE_PROTO_H
 
 struct RMessage*	PARSE_messages(const UCHAR*, size_t);
+struct rem_fmt*		PARSE_msg_format(const UCHAR*, size_t);
 
 #endif // REMOTE_PARSE_PROTO_H

src/remote/parser.cpp

@@ -36,7 +36,7 @@
 #endif
 
 
-static RMessage* parse_error(rem_fmt* format, RMessage* mesage);
+static rem_fmt* parse_format(const UCHAR*& blr, size_t& blr_length);
 
 
 RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
@@ -49,50 +49,129 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
  *
  * Functional description
  *	Parse the messages of a blr request.  For each message, allocate
- *	a message (msg) and a format (fmt) block.  Return the number of
+ *	a message (msg) and a format (fmt) block.
- *	messages found.  If an error occurs, return -1;
  *
  **************************************/
 
-	if (blr_length < 2)
+	if (blr_length < 3)
-		return (RMessage*) -1;
+		return NULL;
-	blr_length -= 2;
+	blr_length -= 3;
 
 	const SSHORT version = *blr++;
 	if (version != blr_version4 && version != blr_version5)
-		return (RMessage*) -1;
+		return NULL;
 
 	if (*blr++ != blr_begin)
-		return 0;
+		return NULL;
 
 	RMessage* message = NULL;
 	ULONG net_length = 0;
 
+	bool error = false;
+
 	while (*blr++ == blr_message)
 	{
-		if (blr_length < 4)
+		if (blr_length-- == 0)
-			return parse_error(0, message);
+		{
-		blr_length -= 4;
+			error = true;
+			break;
+		}
 
 		const USHORT msg_number = *blr++;
+
+		rem_fmt* const format = parse_format(blr, blr_length);
+		if (!format)
+		{
+			error = true;
+			break;
+		}
+
+		RMessage* next = new RMessage(format->fmt_length);
+		next->msg_next = message;
+		message = next;
+		message->msg_address = reinterpret_cast<UCHAR*>(format);
+		message->msg_number = msg_number;
+
+		if (blr_length-- == 0)
+		{
+			error = true;
+			break;
+		}
+	}
+
+	if (error)
+	{
+		for (RMessage* next = message; next; next = message)
+		{
+			message = message->msg_next;
+			delete next->msg_address;
+			delete next;
+		}
+
+		return NULL;
+	}
+
+	return message;
+}
+
+
+rem_fmt* PARSE_msg_format(const UCHAR* blr, size_t blr_length)
+{
+/**************************************
+ *
+ *	P A R S E _ m s g _ f o r m a t
+ *
+ **************************************
+ *
+ * Functional description
+ *	Parse the message of a blr request and return its format.
+ *
+ **************************************/
+
+	if (blr_length < 4)
+		return NULL;
+	blr_length -= 4;
+
+	const SSHORT version = *blr++;
+	if (version != blr_version4 && version != blr_version5)
+		return NULL;
+
+	if (*blr++ != blr_begin)
+		return NULL;
+
+	if (*blr++ != blr_message)
+		return NULL;
+
+	blr++; // skip message number
+
+	return parse_format(blr, blr_length);
+}
+
+static rem_fmt* parse_format(const UCHAR*& blr, size_t& blr_length)
+{
+	if (blr_length < 2)
+		return NULL;
+	blr_length -= 2;
+
 	USHORT count = *blr++;
 	count += (*blr++) << 8;
-		rem_fmt* const format = new rem_fmt(count);
+
-#ifdef DEBUG_REMOTE_MEMORY
+	Firebird::AutoPtr<rem_fmt> format(new rem_fmt(count));
-		printf("PARSE_messages            allocate format  %x\n", format);
+
-#endif
+	ULONG net_length = 0;
 	ULONG offset = 0;
+
 	for (dsc* desc = format->fmt_desc.begin(); count; --count, ++desc)
 	{
 		if (blr_length-- == 0)
-				return parse_error(format, message);
+			return NULL;
 
 		USHORT align = 4;
 		switch (*blr++)
 		{
 		case blr_text:
 			if (blr_length < 2)
-					return parse_error(format, message);
+				return NULL;
 			blr_length -= 2;
 			desc->dsc_dtype = dtype_text;
 			desc->dsc_length = *blr++;
@@ -102,7 +181,7 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
 
 		case blr_varying:
 			if (blr_length < 2)
-					return parse_error(format, message);
+				return NULL;
 			blr_length -= 2;
 			desc->dsc_dtype = dtype_varying;
 			desc->dsc_length = *blr++ + sizeof(SSHORT);
@@ -112,7 +191,7 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
 
 		case blr_cstring:
 			if (blr_length < 2)
-					return parse_error(format, message);
+				return NULL;
 			blr_length -= 2;
 			desc->dsc_dtype = dtype_cstring;
 			desc->dsc_length = *blr++;
@@ -124,7 +203,7 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
 
 		case blr_text2:
 			if (blr_length < 4)
-					return parse_error(format, message);
+				return NULL;
 			blr_length -= 4;
 			desc->dsc_dtype = dtype_text;
 			desc->dsc_scale = *blr++;
@@ -136,7 +215,7 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
 
 		case blr_varying2:
 			if (blr_length < 4)
-					return parse_error(format, message);
+				return NULL;
 			blr_length -= 4;
 			desc->dsc_dtype = dtype_varying;
 			desc->dsc_scale = *blr++;
@@ -148,7 +227,7 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
 
 		case blr_cstring2:
 			if (blr_length < 4)
-					return parse_error(format, message);
+				return NULL;
 			blr_length -= 4;
 			desc->dsc_dtype = dtype_cstring;
 			desc->dsc_scale = *blr++;
@@ -160,7 +239,7 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
 
 		case blr_short:
 			if (blr_length-- == 0)
-					return parse_error(format, message);
+				return NULL;
 			desc->dsc_dtype = dtype_short;
 			desc->dsc_length = sizeof(SSHORT);
 			desc->dsc_scale = *blr++;
@@ -169,7 +248,7 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
 
 		case blr_long:
 			if (blr_length-- == 0)
-					return parse_error(format, message);
+				return NULL;
 			desc->dsc_dtype = dtype_long;
 			desc->dsc_length = sizeof(SLONG);
 			desc->dsc_scale = *blr++;
@@ -178,7 +257,7 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
 
 		case blr_int64:
 			if (blr_length-- == 0)
-					return parse_error(format, message);
+				return NULL;
 			desc->dsc_dtype = dtype_int64;
 			desc->dsc_length = sizeof(SINT64);
 			desc->dsc_scale = *blr++;
@@ -187,7 +266,7 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
 
 		case blr_quad:
 			if (blr_length-- == 0)
-					return parse_error(format, message);
+				return NULL;
 			desc->dsc_dtype = dtype_quad;
 			desc->dsc_length = sizeof(SLONG) * 2;
 			desc->dsc_scale = *blr++;
@@ -219,7 +298,7 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
 		case blr_blob2:
 			{
 				if (blr_length < 4)
-						return parse_error(format, message);
+					return NULL;
 				blr_length -= 4;
 				desc->dsc_dtype = dtype_blob;
 				desc->dsc_length = sizeof(SLONG) * 2;
@@ -259,42 +338,24 @@ RMessage* PARSE_messages(const UCHAR* blr, size_t blr_length)
 			break;
 
 		default:
-				fb_assert(FALSE);
+			fb_assert(false);
-				return parse_error(format, message);
+			return NULL;
 		}
+
 		if (desc->dsc_dtype == dtype_varying)
 			net_length += 4 + ((desc->dsc_length - 2 + 3) & ~3);
 		else
 			net_length += (desc->dsc_length + 3) & ~3;
+
 		if (align > 1)
 			offset = FB_ALIGN(offset, align);
+
 		desc->dsc_address = (UCHAR*) (IPTR) offset;
 		offset += desc->dsc_length;
 	}
+
 	format->fmt_length = offset;
 	format->fmt_net_length = net_length;
-		RMessage* next = new RMessage(format->fmt_length);
-#ifdef DEBUG_REMOTE_MEMORY
-		printf("PARSE_messages            allocate message %x\n", next);
-#endif
-		next->msg_next = message;
-		message = next;
-		message->msg_address = reinterpret_cast<UCHAR*>(format);
-		message->msg_number = msg_number;
-	}
 
-	return message;
+	return format.release();
-}
-
-
-static RMessage* parse_error(rem_fmt* format, RMessage* message)
-{
-	delete format;
-	for (RMessage* next = message; next; next = message)
-	{
-		message = message->msg_next;
-		delete next->msg_address;
-		delete next;
-	}
-	return (RMessage*) -1;
 }

src/remote/protocol.cpp

@@ -1544,14 +1544,7 @@ static bool_t xdr_sql_blr(XDR* xdrs,
 		// setting up a format
 
 		if (blr->cstr_length)
-		{
+			*fmt_ptr = PARSE_msg_format(blr->cstr_address, blr->cstr_length);
-			RMessage* temp_msg = (RMessage*) PARSE_messages(blr->cstr_address, blr->cstr_length);
-			if (temp_msg != (RMessage*) -1)
-			{
-				*fmt_ptr = (rem_fmt*) temp_msg->msg_address;
-				delete temp_msg;
-			}
-		}
 	}
 
 	// If we know the length of the message, make sure there is a buffer
@@ -1781,8 +1774,6 @@ static bool_t xdr_trrq_blr(XDR* xdrs, CSTRING* blr)
 	procedure->rpr_out_format = NULL;
 
 	RMessage* message = PARSE_messages(blr->cstr_address, blr->cstr_length);
-	if (message != (RMessage*) -1)
-	{
 	while (message)
 	{
 		switch (message->msg_number)
@@ -1810,9 +1801,6 @@ static bool_t xdr_trrq_blr(XDR* xdrs, CSTRING* blr)
 			break;
 		}
 	}
-	}
-	else
-		fb_assert(FALSE);
 
 	return TRUE;
 }