Valgrind: Invalid write of size 1 on a strcpy

Question

The code is attempting to serialize a large uint8_t array into a binary character stream. I should have plenty of room in the buffer to copy this data; why am I getting these errors? Posted is a complete runnable code with the output error. The valgrind command used is:

valgrind --leak-check=yes --track-origins=yes ./prog

Main:

#include "string.h"
#include "stdio.h"
#include "stdlib.h"
#include "stdint.h"
#include <unistd.h>

#define MAX_FRAME_SIZE 9236
#define DAT_LEN 8202

void create_bit_stream(uint8_t frame[], size_t frame_size, char *frame_bits)
{
  /*
   * Takes in uint8_t array of data and converts to binary stream of characters
   * frame        : input array of uint8_t type data
   * frame_size   : size_t length of frame --> sizeof(frame)/sizeof(frame[0])
   * frame_bits   : pointer for returning the character string
   *
   * RETURNS string of binary values generated from input array
  */

  uint8_t tmp[frame_size];
  memset(tmp,0,frame_size);
  memcpy(tmp,frame,frame_size);
  // char bit_string[frame_size*8+1];
  char *bit_string = malloc(frame_size*8+1);
  memset(bit_string,'\0',frame_size*8 + 1);
  for (int i = 0; i < frame_size; i++)
  {
    for (int j = 0; j < 8; j++)
    {
      bit_string[i*8+j] = '0' + (tmp[i] >> 7);
      tmp[i] <<= 1;
    }
  }
  bit_string[frame_size*8] = '\0';
  strcpy(frame_bits, bit_string);
  free(bit_string);
  return;

}

int main(int argc, char *argv[])
{

 char *frame_bits = malloc(MAX_FRAME_SIZE + 1);
 if(!frame_bits)
 {exit(EXIT_FAILURE);}
 memset(frame_bits,'\0',MAX_FRAME_SIZE + 1);

 uint8_t *DAT_frame = malloc(DAT_LEN);
 if(!DAT_frame)
 {exit(EXIT_FAILURE);}
 memset(DAT_frame,0,DAT_LEN);

 //fill with arbitrary data
 for(int i = 0; i < DAT_LEN; i++)
 {
   DAT_frame[i] = i % 255;
 }

 create_bit_stream(DAT_frame, DAT_LEN, frame_bits);

 free(frame_bits); free(DAT_frame);
 return 0;
}

Error:

==2994== Invalid write of size 1
==2994==    at 0x4C31060: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2994==    by 0x40088B: create_bit_stream (stack.c:36)
==2994==    by 0x400993: main (stack.c:61)
==2994==  Address 0x5205455 is 0 bytes after a block of size 9,237 alloc'd
==2994==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2994==    by 0x4008CE: main (stack.c:45)
==2994== 
==2994== Source and destination overlap in strcpy(0x5203040, 0x52074f0)
==2994==    at 0x4C310E6: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2994==    by 0x40088B: create_bit_stream (stack.c:36)
==2994==    by 0x400993: main (stack.c:61)

Answer 1

Your create_bit_stream takes a stream of bytes and expands it to a stream of ASCII-coded bits. That is, if you feed it byte 0x42 ('B') it will produce the string "01000010", which is nine bytes long (counting the terminating nul). You forgot to take this into account when you allocated space for frame_bits -- not in create_bit_stream, but in main:

char *frame_bits = (char *)malloc((MAX_FRAME_SIZE + 1)*sizeof(char));

should read

char *frame_bits = malloc(MAX_FRAME_SIZE * 8 + 1);

(Don't cast the result of malloc.)

(As I pointed out in the comments on the question, sizeof(char) == 1 and sizeof(uint8_t) == 1 by definition; explicitly writing either is redundant and a bad code smell.)

(APIs like these, where the caller supplies a variable-length buffer that the callee writes into, should always pass in the size of the output buffer. Your design is dangerous for the same reason that gets and sprintf are dangerous.)