Segmentation fault due to string copy

Question

In the code below the code crashes after executing while loop for 4th time, and it crashes in strcpy

strcpy(userver_info[location].Tag_Name, field);

When debugging it gave following error

Can't find a source file at "strcpy-sse2-unaligned.S"

I am not able to figureout what is the issue which is causing crash here, after facing the above issue i did structure padding thinking that may be the issue and even that is not working out, can anyone please tell me why is the code crashing.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>


#define BUFFER_SIZE               1024
#define NUM_OF_UNDERLYING_SERVER  0


#define SHM_ADDR_LEN              15
#define SERVER_NAME_LEN           25
#define TAG_NAME_LEN              25
#define TAG_DESC_LEN              30
#define ACCESS_LEVEL_LEN          5
#define DATA_TYPE_NAME_LEN            10

typedef struct {
  char    SHM_Location[SHM_ADDR_LEN];
  char    Server_ID[SERVER_NAME_LEN];
  char    Tag_Name[TAG_NAME_LEN];
  int     Tag_ID;
  char    Tag_Description[TAG_DESC_LEN];
  char    Access_Level[ACCESS_LEVEL_LEN];
  char    Data_Type[DATA_TYPE_NAME_LEN];
}u_server_info;

typedef struct {
  int num_of_tags;
  u_server_info *underlying_server_data;
}opcua_u_server;



void parse_underlying_opcua_server(char * buffer, u_server_info *userver_info) {

  static int location = 0;
  char *field;
  /* get SHM_Location */
  field = strtok(buffer,",");
  strcpy(userver_info[location].SHM_Location, field);

  /* get Server_ID */
  field=strtok(NULL,",");
  strcpy(userver_info[location].Server_ID, field);

  /* get Tag_Name */
  field = strtok(NULL,",");
  strcpy(userver_info[location].Tag_Name, field);

  /* get Tag_ID */
  field=strtok(NULL,",");
  userver_info[location].Tag_ID = atoi(field);

  /* get Tag_Description */
  field = strtok(NULL,",");
  strcpy(userver_info[location].Tag_Description, field);

  /* get Access_Level */
  field = strtok(NULL,",");
  strcpy(userver_info[location].Access_Level, field);

  /* get Data_Type */
  field = strtok(NULL,",");
  strcpy(userver_info[location].Data_Type, field);

  /* display the result */
  printf("%s\t %s\t %s\t %d\t %s\t %s\t %s\n",
          userver_info[location].SHM_Location,
          userver_info[location].Server_ID,
          userver_info[location].Tag_Name,
          userver_info[location].Tag_ID,
          userver_info[location].Tag_Description,
          userver_info[location].Access_Level,
          userver_info[location].Data_Type);

  location++;
}


int get_line_count(char *filename) {

  FILE *fp;
  int line_count = 0;
  char c;
  fp = fopen(filename, "r");
  if (fp == NULL){
      printf("Unable to open file '%s' to count lines\n",filename);
      return -1;
  }
  for (c = getc(fp); c != EOF; c = getc(fp))
      if (c == '\n') // Increment count if this character is newline
          line_count = line_count + 1;
  fclose(fp);
  return line_count;
}

int main(int argc, char **argv)
{
  char filename[] = "underlying_server.csv";
  char buffer[BUFFER_SIZE];
  FILE *fp;
  int tags_count = 0;
  opcua_u_server *ua_server = NULL;

  /* open the CSV file */
  fp = fopen(filename,"r");
  if( fp == NULL) {
      printf("Unable to open file '%s'\n",filename);
      exit(EXIT_FAILURE);
  }

  tags_count = get_line_count(filename);
  /* Allocate memmory for each Underlying server */
  ua_server= malloc(sizeof(opcua_u_server));
  if (ua_server == NULL) {
      printf("Malloc failed");
      exit(EXIT_FAILURE);
  }
  /* Allocate memory for each underlying server tag information */
  ua_server[NUM_OF_UNDERLYING_SERVER].underlying_server_data =
          malloc(tags_count * sizeof(ua_server->underlying_server_data));

  if (ua_server[NUM_OF_UNDERLYING_SERVER].underlying_server_data == NULL) {
      printf("Malloc failed");
      exit(EXIT_FAILURE);
  }
  /* process the data */
  while(fgets(buffer,BUFFER_SIZE,fp)) {

      parse_underlying_opcua_server(buffer,
                      ua_server[NUM_OF_UNDERLYING_SERVER].underlying_server_data);

  }
  ua_server->num_of_tags = tags_count;
  /*Reset the value for next server*/
  tags_count = 0;
  /* close file */
  fclose(fp);

  return(0);
}

My underlying_server.csv is like this

1000,Server_1,Tag_1,3000,Created_tag_3000,AR,BOOL
1001,Server_1,Tag_2,3001,Created_tag_3001,AR,BOOL
1002,Server_1,Tag_3,3002,Created_tag_3002,AR,BOOL
1003,Server_1,Tag_4,3003,Created_tag_3003,AR,BOOL
1004,Server_1,Tag_5,3004,Created_tag_3004,AR,REAL
1005,Server_1,Tag_6,3005,Created_tag_3005,AR,REAL
1006,Server_1,Tag_7,3006,Created_tag_3006,AW,REAL
1007,Server_1,Tag_8,3007,Created_tag_3007,AW,REAL
1008,Server_1,Tag_9,3008,Created_tag_3008,AW,REAL
1009,Server_1,Tag_10,3009,Created_tag_3009,AW,REAL
1010,Server_1,Tag_11,3010,Created_tag_3010,AW,REAL
1011,Server_1,Tag_12,3011,Created_tag_3011,AW,DWORD
1012,Server_1,Tag_13,3012,Created_tag_3012,AW,DWORD
1013,Server_1,Tag_14,3013,Created_tag_3013,AR,DWORD
1014,Server_1,Tag_15,3014,Created_tag_3014,AR,DWORD
1015,Server_1,Tag_16,3015,Created_tag_3015,AR,DWORD
1016,Server_1,Tag_17,3016,Created_tag_3016,AR,DWORD
1017,Server_1,Tag_18,3017,Created_tag_3017,AR,DWORD
1018,Server_1,Tag_19,3018,Created_tag_3018,AR,DWORD
1019,Server_1,Tag_20,3019,Created_tag_3019,AR,DWORD
1020,Server_1,Tag_21,3020,Created_tag_3020,AR,DWORD
1021,Server_1,Tag_22,3021,Created_tag_3021,AW,BOOL
1022,Server_1,Tag_23,3022,Created_tag_3022,AW,BOOL
1023,Server_1,Tag_24,3023,Created_tag_3023,AW,BOOL
1024,Server_1,Tag_25,3024,Created_tag_3024,AW,BOOL
1025,Server_1,Tag_26,3025,Created_tag_3025,AW,BOOL
1026,Server_1,Tag_27,3026,Created_tag_3026,AW,DWORD
1027,Server_1,Tag_28,3027,Created_tag_3027,AR,DWORD
1028,Server_1,Tag_29,3028,Created_tag_3028,AR,DWORD
1029,Server_1,Tag_30,3029,Created_tag_3029,AR,DWORD
1030,Server_1,Tag_31,3030,Created_tag_3030,AR,REAL

That looks like way too much code; can you reduce it further to a [mcve]? — Angew is no longer proud of SO, May 07 '19 at 11:10
Start with `char c;` -> `int c;` for `for (c = getc(fp); c != EOF; c = getc(fp))`. `getc` returns an `int`. — Jabberwocky, May 07 '19 at 11:13
For starters, check if the `field` values up to and including that point are all what you expect - in particular, if `strtok` just returned a null pointer, that's bad news. — aschepler, May 07 '19 at 11:15
Your malloc code doesn't make any sense. In addition to being completely wrong, you should be [correctly allocating multi-dimensional arrays](https://stackoverflow.com/questions/42094465/correctly-allocating-multi-dimensional-arrays) anyway. — Lundin, May 07 '19 at 11:20
@Angew This is fine, the bug was easy to spot in a few minutes of untangling what `userver_info` was. Just need a bit of text search to do that, so it doesn't matter if this is 100 LoC or 100k LoC. — Lundin, May 07 '19 at 11:21
@Angew What the OP posted looks like a MCVE to me. Maybe not 100% minimal, but other than that, it's fine. — Nikos C., May 07 '19 at 11:27
@NikosC. Yeah, the "Minimal" part is what I was after. Note that I neither DVed nor VTCed, it just looked like an unnecessary wall of code to me. — Angew is no longer proud of SO, May 07 '19 at 11:51

score 5 · Accepted Answer · answered May 07 '19 at 11:16

5

I'm sure there is a dup for this, you are allocating the second buffer based on the sizeof the pointer, not the object being pointed to:

/* Allocate memory for each underlying server tag information */
ua_server[NUM_OF_UNDERLYING_SERVER].underlying_server_data =
      malloc(tags_count * sizeof(ua_server->underlying_server_data));

becomes:

/* Allocate memory for each underlying server tag information */
ua_server[NUM_OF_UNDERLYING_SERVER].underlying_server_data =
      malloc(tags_count * sizeof(*(ua_server->underlying_server_data)));

Note the actual code *(ua_server->underlying_server_data is bad at this moment, but sizeof is compile-time and is just using the code as a template to get the object size.

I sometimes wish there was a compiler warning for "You asked for the sizeof a pointer"

answered May 07 '19 at 11:16

Gem Taylor

5,381
1
9
27

You can just write `sizeof(*ua_server->underlying_server_data)`. You don't need the parentheses. – Nikos C. May 07 '19 at 11:22
`ua_server= malloc(sizeof(opcua_u_server));` is also broken because it only allocates a single object, but the code then uses it as an array of pointers, which it is not. All malloc lines in the code are wrong. – Lundin May 07 '19 at 11:26
You can just write sizeof *ua_server->underlying_server_data. You don't need the parentheses. – dmuir May 07 '19 at 11:46
@dmur you can, but personally I find it clearer with the brackets when I switch association sides. – Gem Taylor May 07 '19 at 12:49

datenwolf · Answer 2 · 2019-05-07T12:16:46.330

Never use strcpy, it's insecure, because it doesn't perform any form of bounds constraining and will just soldier on copying until it hits a 0 byte in the source buffer. If the source buffer isn't (properly) 0-terminated, or the contents of the source buffer are too long to fit into the destination, it will create undefined behavior (crash or be vulnerable to attack).

Putting aside the problem with fixed size fields, it is absolutely necessary to check the size limits of buffer operations. The standard function you'd use (although it still isn't perfect) is strncpy (note the extra n). The caveat is, that strncpy will omit the terminating 0 byte if it doesn't fit into the destination.

Since your fields are fixed size you can use it like this

strncpy(userver_info[location].SHM_Location, field, SHM_ADDR_LEN-1);
userver_info[location].SHM_Location[SHM_ADDR_LEN-1] = 0;

i.e. it copies up to SHM_ADDR_LEN-1 bytes and will always put a terminating 0 at the very end of the buffer.

Segmentation fault due to string copy

2 Answers2