Why is my variable corrupt after returning from a function?

Question

Below is my c code:

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

char *username;
char *password;

void set_credentials(char*, char*);

int main(void)
{
  set_credentials();
  printf("%s\n", username);     //look here 3
  printf("%s\n", password);     //look here 4
  return EXIT_SUCCESS;
}

void set_credentials(char *username, char *password)
{
  char c;
  char lines[2][100];
  char * tmp = * lines;
  char * user = "user";
  int i = 0;
  FILE *fp = fopen("/netnfork/config/netnfork_credentials.properties", "r");

  if (fp == NULL)
    exit(EIO);
  while ((c = fgetc(fp)) != EOF)
  {
    if (c != '\n')
    {
      *tmp = c;
      tmp++;
    } else {
      *tmp = '\0';
      i++;
      tmp = lines[i];
    }
  }
  fclose(fp);
  i = 0;
  while (i < 2)
  {
    if (strncmp(user, lines[i], 4) == 0)
    {
      username = lines[i] + 5;
      printf("%s\n", username); //look here 1
    } else {
      password = lines[i] + 9;
      printf("%s\n", password);  //look here 2
    }
    i++;
  }
}

Now, when I run the code, I get this:

myname //for 1
mypassword //for 2
myname // for 3
mypasswo�� // for 4

I can't understand why that behavior. Have anyone ideea about why that?

uswername/password point to variables that are allocated on the stack and are destroyed when the set_credentials function returns — Gir
– Gir, Commented Sep 3, 2012 at 12:31
Indeed. My bad. I'm not using to bad coding style anymore... XD — Eregrith
– Eregrith, Commented Sep 3, 2012 at 12:38
Now you modified your code, you are calling set_credentials without any parameters whereas you declared two, you will have compiler warnings. — Eregrith
– Eregrith, Commented Sep 3, 2012 at 12:51

David · Accepted Answer · 2012-09-03 12:35:10Z

2

Your problem is that you are assigning username and password to parts of the lines array. The lines array is a local variable and the memory allocated to it is no longer safe after set_credentials finishes. To fix it, use malloc to get the memory for your username and password, or make lines a global variable like username and password are.

answered Sep 3, 2012 at 12:35

David

1,4198 silver badges8 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Scooter · Accepted Answer · 2012-09-04 15:11:19Z

1

Here is a version that uses malloc() and free().

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


/* "user " */
#define USERLINE_PREFIX 5
/* "password " */
#define PASSLINE_PREFIX 9



void set_credentials(char**, char**);

int main(void)
{
  /* to point to the username and password obtained from a text file */
  char *username = NULL;
  char *password = NULL;
  set_credentials(&username,&password);
  /* (original scheme) printf("username: %s\n", username);  */
  /* (original scheme) printf("password: %s\n", password);  */
  printf("username: %s\n", username + USERLINE_PREFIX ); // line starts "user "    
  printf("password: %s\n", password + PASSLINE_PREFIX ); // line starts "password "
  /* (original scheme)  free(username - USERLINE_PREFIX); */
  /* (original scheme) free(password - PASSLINE_PREFIX); */
  free(password);
  free(username);
  return EXIT_SUCCESS;
}

void set_credentials(char **username, char **password)
{
  /* file format:
  line 1 ->user <username>
  line 2 ->password <password>
  */
  char c;
  #define FILE_LINES 2
  #define MAX_LINE_LENGTH 100
  int i = 0, j = 0;
  char *lines[FILE_LINES];
  char *tmp = NULL;
  char *user = "user ";
  char *pass = "password ";
  char user_found = 0, password_found = 0;
  for (j = 0; j < FILE_LINES; j++)
  {
    lines[j] = malloc( MAX_LINE_LENGTH + 1 );
    lines[j][0] = '\0';
  }
  tmp = lines[0];
  const char *filename = "/netnfork/config/netnfork_credentials.properties";
  FILE *fp = fopen(filename, "r");

  if (fp == NULL)
  {
    printf("ERROR %d trying to open %s\n",errno,filename);
    /* if not exiting program, would need to free() here */
    exit(EIO);
  }
                                /* in case more lines than expected */
  while ((c = fgetc(fp)) != EOF && i < FILE_LINES)
  {
    if (c != '\n')
    {
      *tmp = c;
      tmp++;
    } else {
      *tmp = '\0';
      i++;
      tmp = lines[i];
    }
  }
  if ( i < 2 )  {
     printf("ERROR: file %s is incomplete needs %d lines (password and user)\n",filename,FILE_LINES);
     /* if not exiting program, would need to free() here */
     exit(1);
  }
  fclose(fp);
  i = 0;
  while (i < FILE_LINES)
  {
    if (strncmp(user, lines[i], USERLINE_PREFIX) == 0)
    {
       user_found = 1;
       /* (original scheme) *username = lines[i] + USERLINE_PREFIX; */
       *username = lines[i];
    }
    else if  ( strncmp (pass, lines[i],PASSLINE_PREFIX) == 0 ) {
       password_found = 1;
       /* (original scheme) *password = lines[i] + PASSLINE_PREFIX; */
       *password = lines[i];
    }
    else {
       printf("ERROR: invalid line in file:\n");
       printf("%s\n",lines[i]);
       /* if not exiting program, would need to free() here */
       exit(1);
    }
    i++;
  }
  /* check for the extremely unlikely event that the two lines are both of the same type */
  if ( ! (password_found && user_found ) )
  {
     printf("ERROR: file %s is invalid, missing %s line\n",filename, (user_found) ? "password" : "user" );  
     /* if not exiting program, would need to free() here */
     exit(1);
  }
}

edited Sep 4, 2012 at 15:11

answered Sep 4, 2012 at 8:44

Scooter

7,1118 gold badges46 silver badges73 bronze badges

6 Comments

artaxerxe Over a year ago

I just read some documentation related to free() function, and here I have a misunderstanding: When you call free() on a pointer, it (the free function) will make good for reuse the whole memory allocated to that pointer (through malloc). So, doing this: free(username - USERLINE_PREFIX);, wouldn't it made unreliable the data stored in the username?

Scooter Over a year ago

@artaxerxe The problem with this code is that the actual character array is "user my_userid\0..................... (up to 101 bytes)". But the function does not return a pointer to the start of the line, it returns a pointer to the 6th character in the line. So if free() is going to handle all the memory malloc()ed, it must be given an address that is less than where the "char *" is currently pointing. I will change the code to return a pointer to the start (and then main must be modified to take from 6th character) so free will work without pointer arithmetic being necessary.

Scooter Over a year ago

@artaxerxe Ideally, the function would have extra logic to remove the "user " and "password " prefix part and only store the actual user name and password in the malloc()ed memory.

artaxerxe Over a year ago

I understood this. But I think so: when you pass the initial pointer to array (username - USERLINE_PREFIX), the free function will free the whole array ("user my_user\0......"). So, doing that, the data from *username that is in main will be unreliable (taking in account that free clears the whole initial array where *username points to the 6th element).

Scooter Over a year ago

@artaxerxe Right, you should not access any data that was in an area created by malloc() after free() is called. But I don't think that free() clears the memory (sets the bytes to 0), it just marks it as available for use. If you accidentally reference that memory it could still have the same values as before the free(), up until such point that it gets overwritten by another call to malloc() and subsequent setting of data.

|

Tony The Lion · Accepted Answer · 2012-09-03 12:34:55Z

1

You have created two pointers username and password which point to function local variables created on that particulars functions stack frame. When the function returns, these pointers will point to garbage, since the stack frame has been destroyed and so the function local variables.

You need to pass the variables as args to your set_credentials function or you need create a struct which you can return from your function filled up properly. Return the struct by value. No need for pointers.

answered Sep 3, 2012 at 12:34

Tony The Lion

63.6k71 gold badges252 silver badges424 bronze badges

1 Comment

artaxerxe Over a year ago

If you look on my question, it is modified with username and password as parameters to set_credentials. But it gets me error when run: Segmentation fault (core dumped)

Scooter · Accepted Answer · 2012-09-03 14:38:27Z

1

David identifies your problem. One solution is to allocate two character arrays that the function can populate.

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



void set_credentials(char*, char*);

int main(void)
{
  /* to hold the username and password obtained from a text file */
  char username[200] = { '\0' };
  char password[200] = { '\0' };
  set_credentials(username,password);
  printf("%s\n", username);     //look here 3
  printf("%s\n", password);     //look here 4
  return EXIT_SUCCESS;
}

void set_credentials(char *username, char *password)
{
  char c;
  char lines[2][100];
  int i = 0;
  char *tmp = *lines;
  char *user = "user";
  char *pass = "password";


  const char *filename = "/netnfork/config/netnfork_credentials.properties";
  FILE *fp = fopen(filename, "r");

  if (fp == NULL)
    exit(EIO);
  else
    printf("successfully opened %s\n",filename);
                                /* in case more than 2 lines */
                                /* or increase array first dimension */ 
  while ((c = fgetc(fp)) != EOF && i < 2)
  {
    if (c != '\n')
    {
      *tmp = c;
      tmp++;
    } else {
      *tmp = '\0';
      i++;
      tmp = lines[i];
    }
  }

  fclose(fp);
  i = 0;
  while (i < 2)
  {
    if (strncmp(user, lines[i], 4) == 0)
    {
      strcpy(username,lines[i] + 5);
    }
    else if  ( strncmp (pass, lines[i],8) == 0 ) {
      strcpy(password,lines[i] + 9);
    }
    else {
      /* can't assume file is correct so fatal error if not */
      printf("ERROR: invalid line in file:\n");
      printf("%s\n",lines[i]);
      exit(1);
    }
    i++;
  }
}

edited Sep 3, 2012 at 14:38

answered Sep 3, 2012 at 13:49

Scooter

7,1118 gold badges46 silver badges73 bronze badges

1 Comment

artaxerxe Over a year ago

Thanks for your clear answer! Anyway, just as a case study, is it possible to get this functioning without copy (strcpy function), only through pointers (and without global variables, of course). I come from Java and this issue confused me a lot. I only want to see the real behavior of local variables in C.

Scooter · Accepted Answer · 2012-09-04 07:50:22Z

You can use a static variable in the function which allows you to return a pointer to it, since a static variable will be around even after the function has exited.

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


/* 
  In order to modify a caller's char in a function it must be passed as pointer to char 
  In order to modify a caller's "char *" in a function it must be passed as pointer to "char *" 
*/ 
void set_credentials(char**, char**);

int main(void)
{
  /* to point to the username and password obtained from a text file */
  char *username = NULL;
  char *password = NULL;
  /* pass the addresses of the char *'s so their contents can be modified */
  set_credentials(&username,&password);
  printf("%s\n", username);     //look here 3
  printf("%s\n", password);     //look here 4
  return EXIT_SUCCESS;
}

void set_credentials(char **username, char **password)
{
  char c;
#define MAX_FILE_LINES 2
  static char lines[MAX_FILE_LINES][100];
  int i = 0;
  char *tmp = *lines;
  char *user = "user";
  char *pass = "password";


  const char *filename = "/netnfork/config/netnfork_credentials.properties";
  FILE *fp = fopen(filename, "r");

  if (fp == NULL)
  {
    printf("ERROR: unable to open %s\n",filename);
    exit(EIO);
  }
  else
    printf("successfully opened %s\n",filename);
                                /* in case more lines than expected */
  while ((c = fgetc(fp)) != EOF && i < MAX_FILE_LINES)
  {
    if (c != '\n')
    {
      *tmp = c;
      tmp++;
    } else {
      *tmp = '\0';
      i++;
      tmp = lines[i];
    }
  }

  fclose(fp);
  i = 0;
  while (i < 2)
  {
    if (strncmp(user, lines[i], 4) == 0)
    {
      /* assign to the char * back in the calling routine */
      *username = lines[i] + 5;
    }
    else if  ( strncmp (pass, lines[i],8) == 0 ) {
      /* assign to the char * back in the calling routine */
      *password = lines[i] + 9;
    }
    else {
      /* can't assume file is correct so fatal error if not */
      printf("ERROR: invalid line in file:\n");
      printf("%s\n",lines[i]);
      exit(1);
    }
    i++;
  }
}

That's the solution that I was looking for. Now I think (at least I hope) that understood how automatic variables work in C. :)

Collectives™ on Stack Overflow

Why is my variable corrupt after returning from a function?

5 Answers 5

Comments

6 Comments

1 Comment

1 Comment

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

Comments

6 Comments

1 Comment

1 Comment

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related