[OpenOCD-devel] [PATCH]: a77a084 gdb_server: fix GDB_BUFFER_SIZE usage, fix unaligned access during bulk transfers

[<ge...@op...>]

This is an automated email from Gerrit.

Bohdan Tymkiv (bh...@cy...) just uploaded a new patch set to Gerrit, which you can find at http://openocd.zylin.com/5109

-- gerrit

commit a77a0840c4bef4002d80c44e5a1fc812b4a548ad
Author: Bohdan Tymkiv <bh...@cy...>
Date:   Tue Apr 16 17:28:29 2019 +0300

    gdb_server: fix GDB_BUFFER_SIZE usage, fix unaligned access during bulk transfers
    
    Currently size of the GDB buffer is 16384 bytes but it is treated as
    nul-terminated string in most of the code, so effective size of the
    buffer is actually 16383 bytes. OpenOCD responds with `PacketSize=3fff`
    to qSupported request. Result of GDB's `m` command is encoded in hex so
    each data byte uses two bytes in the buffer. As a result GDB will split
    bulk read requests into chunks 0x1fff bytes each. This causes troubles
    on targets (or memory regions) which support only aligned, word-sized
    access (such as MMIO buffers).
    
    Steps to reproduce (psoc6 target):
    gdb> dump binary memory dump.bin 0x040320000 (0x040320000 + 65536)
    
    OpenOCD:
    Error: Failed to read memory at 0x40321ffe
    Error: Failed to read memory at 0x40321000
    Error: Failed to read memory at 0x40323000
    Error: Failed to read memory at 0x40325ffe
    Error: Failed to read memory at 0x40329ffa
    Error: Failed to read memory at 0x40329ffc
    Error: Failed to read memory at 0x4032bffc
    Error: Failed to read memory at 0x4032dffa
    
    Consolidate GDB_BUFFER_SIZE usage: ensure size of each buffer is
    (GDB_BUFFER_SIZE + 1), add explicit comment that additional byte is used
    for nul-termination. Report correct size of the buffer to GDB (0x4000)
    as recommended in GDB's docummentation: `if the stub stores packets in a
    NUL-terminated format, it should allow an extra byte in its buffer for
    the NUL`
    
    Checked with clang-asan, clang-analyzer, valgrind - no new errors.
    
    Change-Id: I909e8a2c6b010c5d4a304641808d4a807a4ec18d
    Signed-off-by: Bohdan Tymkiv <bh...@cy...>

diff --git a/src/rtos/nuttx.c b/src/rtos/nuttx.c
index 61fd9aa..8c3076e 100644
--- a/src/rtos/nuttx.c
+++ b/src/rtos/nuttx.c
@@ -174,7 +174,7 @@ static int rcmd_offset(const char *cmd, const char *name)
 static int nuttx_thread_packet(struct connection *connection,
 	char const *packet, int packet_size)
 {
-	char cmd[GDB_BUFFER_SIZE / 2] = "";
+	char cmd[GDB_BUFFER_SIZE / 2 + 1] = ""; /* Extra byte for nul-termination */
 
 	if (!strncmp(packet, "qRcmd", 5)) {
 		size_t len = unhexify((uint8_t *)cmd, packet + 6, sizeof(cmd));
diff --git a/src/rtos/rtos.c b/src/rtos/rtos.c
index da0a503..20e875d 100644
--- a/src/rtos/rtos.c
+++ b/src/rtos/rtos.c
@@ -223,7 +223,7 @@ int rtos_qsymbol(struct connection *connection, char const *packet, int packet_s
 	int rtos_detected = 0;
 	uint64_t addr = 0;
 	size_t reply_len;
-	char reply[GDB_BUFFER_SIZE], cur_sym[GDB_BUFFER_SIZE / 2] = "";
+	char reply[GDB_BUFFER_SIZE + 1], cur_sym[GDB_BUFFER_SIZE / 2 + 1] = ""; /* Extra byte for nul-termination */
 	symbol_table_elem_t *next_sym = NULL;
 	struct target *target = get_target_from_connection(connection);
 	struct rtos *os = target->rtos;
diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c
index 95e6c04..9adf2c3 100644
--- a/src/server/gdb_server.c
+++ b/src/server/gdb_server.c
@@ -67,7 +67,7 @@ struct target_desc_format {
 
 /* private connection data for GDB */
 struct gdb_connection {
-	char buffer[GDB_BUFFER_SIZE];
+	char buffer[GDB_BUFFER_SIZE + 1]; /* Extra byte for nul-termination */
 	char *buf_p;
 	int buf_cnt;
 	int ctrl_c;
@@ -1407,8 +1407,6 @@ static int gdb_error(struct connection *connection, int retval)
 
 /* We don't have to worry about the default 2 second timeout for GDB packets,
  * because GDB breaks up large memory reads into smaller reads.
- *
- * 8191 bytes by the looks of it. Why 8191 bytes instead of 8192?????
  */
 static int gdb_read_memory_packet(struct connection *connection,
 		char const *packet, int packet_size)
@@ -2614,7 +2612,7 @@ static int gdb_query_packet(struct connection *connection,
 			&pos,
 			&size,
 			"PacketSize=%x;qXfer:memory-map:read%c;qXfer:features:read%c;qXfer:threads:read+;QStartNoAckMode+;vContSupported+",
-			(GDB_BUFFER_SIZE - 1),
+			GDB_BUFFER_SIZE,
 			((gdb_use_memory_map == 1) && (flash_get_bank_count() > 0)) ? '+' : '-',
 			(gdb_target_desc_supported == 1) ? '+' : '-');
 
@@ -3117,7 +3115,7 @@ static void gdb_sig_halted(struct connection *connection)
 static int gdb_input_inner(struct connection *connection)
 {
 	/* Do not allocate this on the stack */
-	static char gdb_packet_buffer[GDB_BUFFER_SIZE];
+	static char gdb_packet_buffer[GDB_BUFFER_SIZE + 1]; /* Extra byte for nul-termination */
 
 	struct target *target;
 	char const *packet = gdb_packet_buffer;
@@ -3140,7 +3138,7 @@ static int gdb_input_inner(struct connection *connection)
 	 * drain the rest of the buffer.
 	 */
 	do {
-		packet_size = GDB_BUFFER_SIZE-1;
+		packet_size = GDB_BUFFER_SIZE;
 		retval = gdb_get_packet(connection, gdb_packet_buffer, &packet_size);
 		if (retval != ERROR_OK)
 			return retval;

--