[hci] Fix semantics of replace_string() to match code comments

The comments for replace_string() state that a successful return
status guarantees that the dynamically allocated string pointer is no
longer NULL (even if it was initially NULL and the replacement string
is NULL or empty).  This is relied upon by readline() to guarantee
that it will always return a non-NULL string if successful.

The code behaviour does not currently match this comment: an empty
replacement string may result in a successful return status even if
the (single-byte) allocation fails.

Fix up the code behaviour to match the comments, and to additionally
ensure that the edit history is filled in even in the event of an
allocation failure.

Signed-off-by: Michael Brown <mcb30@ipxe.org>

1 files changed, 18 insertions, 17 deletions

diff --git a/src/hci/editstring.c b/src/hci/editstring.c
index b83916c..be9ca06 100644
--- a/src/hci/editstring.c
+++ b/src/hci/editstring.c
@@ -59,10 +59,14 @@ static __nonnull void kill_eol ( struct edit_string *string );
  */
 static int insert_delete ( struct edit_string *string, size_t delete_len,
 			   const char *insert_text ) {
-	size_t old_len, max_delete_len, insert_len, new_len;
+	size_t old_len, max_delete_len, move_len, insert_len, new_len;
 	char *buf;
 	char *tmp;
 
+	/* Prepare edit history */
+	string->mod_start = string->cursor;
+	string->mod_end = string->cursor;
+
 	/* Calculate lengths */
 	buf = *(string->buf);
 	old_len = ( buf ? strlen ( buf ) : 0 );
@@ -70,39 +74,36 @@ static int insert_delete ( struct edit_string *string, size_t delete_len,
 	max_delete_len = ( old_len - string->cursor );
 	if ( delete_len > max_delete_len )
 		delete_len = max_delete_len;
+	move_len = ( max_delete_len - delete_len );
 	insert_len = ( insert_text ? strlen ( insert_text ) : 0 );
 	new_len = ( old_len - delete_len + insert_len );
 
+	/* Delete existing text */
+	memmove ( ( buf + string->cursor ),
+		  ( buf + string->cursor + delete_len ), move_len );
+
 	/* Reallocate string, ignoring failures if shrinking */
 	tmp = realloc ( buf, ( new_len + 1 /* NUL */ ) );
 	if ( tmp ) {
 		buf = tmp;
 		*(string->buf) = buf;
-	} else if ( new_len > old_len ) {
+	} else if ( ( new_len > old_len ) || ( ! buf ) ) {
 		return -ENOMEM;
 	}
 
-	/* Fill in edit history */
-	string->mod_start = string->cursor;
-	string->mod_end = ( ( new_len > old_len ) ? new_len : old_len );
-
-	/* Move data following the cursor */
+	/* Create space for inserted text */
 	memmove ( ( buf + string->cursor + insert_len ),
-		  ( buf + string->cursor + delete_len ),
-		  ( max_delete_len - delete_len ) );
+		  ( buf + string->cursor ), move_len );
 
 	/* Copy inserted text to cursor position */
 	memcpy ( ( buf + string->cursor ), insert_text, insert_len );
 	string->cursor += insert_len;
 
-	/* Terminate string (if not NULL) */
-	if ( buf ) {
-		buf[new_len] = '\0';
-	} else {
-		assert ( old_len == 0 );
-		assert ( insert_len == 0 );
-		assert ( new_len == 0 );
-	}
+	/* Terminate string */
+	buf[new_len] = '\0';
+
+	/* Update edit history */
+	string->mod_end = ( ( new_len > old_len ) ? new_len : old_len );
 
 	return 0;
 }