[SOLVED] C

jmgibson1981 · 03-02-2023, 01:13 PM

It prints the first field just fine. After that though it prints a random letter from the other fields.

Code:

void file_search_func(FILE * file, const char * searchstr,
                      const char * delimiter, char * pointer,
                      int field)
{
  char * token = (char*)calloc(48,sizeof(char));
  char * buffer = (char*)calloc(48,sizeof(char));
  int counter = 0;

  while (fgets(buffer, 48, file)) {
    if (strstr(buffer, searchstr)) {
      token = strtok(buffer, delimiter);
      while (token) {
        if (counter == field) {
          printf("token = %s\n", token);
          break;
        } else {
          counter++;
          token = strtok(NULL, token);
        }
      }
    }
  }
  free(token);
  free(buffer);
}

*EDIT*

Found it. Was doing the token NULL wrong.

Code:

void file_search_func(FILE * file, const char * searchstr,
                      const char * delimiter, char * pointer,
                      const int field)
{
  char * token = (char*)calloc(128,sizeof(char) + 1);
  char * buffer = (char*)calloc(128,sizeof(char) + 1);
  int counter = 0;

  while (fgets(buffer, 128, file)) {
    if (strstr(buffer, searchstr)) {
      printf("buffer = %s\n", buffer);
      token = strtok(buffer, delimiter);
      while (token != NULL) {
        if (counter == field) {
          printf("token = %s\n", token);
          break;
        } else {
          counter++;
          token = strtok(NULL, delimiter);
        }
      }
    }
  }
  free(token);
  free(buffer);
}

astrogeek · 03-02-2023, 02:40 PM

I am not sure I understand how you intend to use this, but your token pointer will likely result in an error and a memory leak.

First, you allocate two blocks of memory:

Code:

char * token = (char*)calloc(128,sizeof(char) + 1);
char * buffer = (char*)calloc(128,sizeof(char) + 1);

Then inside your loop you re-point token to a location inside the buffer block:

Code:

token = strtok(buffer, delimiter);
...
token = strtok(NULL, delimiter);

So at this point the memory originally allocated to token is lost, and token now points to an area inside buffer.

Then you free both:

Code:

free(token);
free(buffer);

If token is not NULL at that point you will produce a double free() error, and either way the memory allocated to token at the top of the function is lost.

Maybe you just want token to be declared as an unallocated char*?

ntubski · 03-02-2023, 04:31 PM

Quote:

Originally Posted by jmgibson1981

Found it. Was doing the token NULL wrong.

I'm not sure exactly what you mean by this, but FYI while (token) is 100% the same as while (token != NULL).

jmgibson1981 · 03-02-2023, 05:29 PM

My goal is to parse a config file by delimiter. This compiles now without error and I think I got the leak. it also does what I want it to, namely return the found field value to main. Thank you for pointing the leak.

Code:

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

// declare functions

void file_search_func(FILE * file, const char * searchstr,
                      const char * delimiter, char ** pointer,
                      const int field);

int main(void)
{
  // set up file and delimiter
  FILE * file;
  file = fopen("testfile.txt", "r");
  char delimiter[1] = ",";
  
  // if file fail to open bail out
  if (!file)
  {
    printf("unable to open file. exiting!\n");
    return(-1);
  }

  // set up pass by reference and return value. free when done
  char * retval;
  file_search_func(file, "lol", delimiter, &retval, 1);
  printf("fail - %s\n", retval);
  free(retval);

  //close and clean up
  fclose(file);
  file = NULL;
}

void file_search_func(FILE * file, const char * searchstr,
                      const char * delimiter, char ** pointer,
                      const int field)
{
  // declare and initialize
  char * token;
  char * buffer = (char*)calloc(24,sizeof(char));
  char * tmp = (char*)malloc(24*sizeof(char));
  int counter = 0;

  // loop until find identifier in string. typically first field
  // tokenize once found
  while (fgets(buffer, 24, file)) {
    if (strstr(buffer, searchstr)) {
      token = strtok(buffer, delimiter);
      while (token) {
        
        // stops when field found of specified string
        // return and clean up
        if (counter == field) {
          printf("token = %s\n", token);
          strcpy(tmp, token);
          *pointer = tmp;
          free(buffer);
          break;
        } else {
          
          // continue to next token until found
          counter++;
          token = strtok(NULL, delimiter);
        }
      }
    }
  }
}

astrogeek · 03-02-2023, 06:18 PM

You should at least initialize retval to NULL, otherwise if fgets() in your function fails or the file is empty or the token is not found you will try to free a random location in memory.

Also, it looks like you may try to use buffer after it has been freed if your input is longer than expected or multi-line.

Buffer may also leak if the function is passed an empty file or if the search is not found or if the field value is greater than the number of tokens in the buffer.

All the little duckies have to be in a row at the same time!

jmgibson1981 · 03-02-2023, 06:28 PM

Ok. I added a free(buffer) and free(tmp) after the main while loop so that cleans that up. i'll null the ptr right now. Thank you much.

astrogeek · 03-02-2023, 09:51 PM

I think you may still have problems with that example.

First, the way you handle the call and retval are confusing to me:

Code:

char * retval;
file_search_func(file, "lol", delimiter, &retval, 1);
printf("fail - %s\n", retval);
free(retval);

As already noted, as written there is no guarantee that file_search_func(...) will set retval, so you may attempt to reference random memory with both the printf(...) function and free(). retval should be initialized and then tested before use.

But the logic here is still confusing because only on a successful search will retval will be set to a found token, but you always report that as a "fail". I cannot tell what you intend with that.

Next, the outer loop in your function may not work as you apparently intend for two reasons:

Code:

while (fgets(buffer, 24, file)) {
  if (strstr(buffer, searchstr)) {
    ...
  }
}

First, fgets() will read a maximum of 23 characters into the buffer, but if the string of tokens is longer than this it may break a token in the middle so that it will never be found by strtok().

Also, as written you may read multiple lines via the enclosing while(){...}, but your field and counter values will lose their most obvious meaning after the first line. Counter will continue to increment and what would field mean in a multi-line stream of tokens?

Next, you test the buffer to see if your search string is found in it, and only if found do you tokenize the buffer string. I infer that you are searching for a specific token and you expect strstr() to test if it is there, then use strtok() to find what field it is in. But strtok() may not produce what strstr() matches. In other words strstr() may match something not returned by strtok(), so the match test seems flawed to me. You should ask yourself what you intended here.

Then, inside your tokenizing loop:

Code:

token = strtok(buffer, delimiter);
while (token) {
  // stops when field found of specified string
  // return and clean up
  if (counter == field) {
  printf("token = %s\n", token);
  strcpy(tmp, token);
  *pointer = tmp;
  free(buffer);
  break;
}

You say it "stops when field found of specified string ... return", which we must take to be your intent, but that is not what it does at all.

It silently tokenizes the buffer until it either reaches the end or until counter == field. But that test is not related at all to the outer searchstr test so it is difficult to see what you might have intended that to do for you.

But you also free the buffer when counter == field, but you only break the inner loop, you do not return, and if there are more characters in the input string you will load them into the buffer you just freed.

My point is to prompt you to think more clearly about how you expect that all to work together by writing a simple spec (i.e. comments in the file or function explaining what it should do), then write the code to perform that purpose under all possible conditions - then look for bugs.

Hope this helps!

jmgibson1981 · 03-04-2023, 01:45 PM

Ok. I need to write comments because it is a bit involved. For reference this is where I am now. It works, at least seems to give me what I want. I've tested it on multiple line files with a few of them having the same identifier. each time it stops after the first line that matches.

Code:

int main()
{
  // declare and initialize
  FILE * file;
  file = fopen("datafile.txt", "r");

  // set up pass by reference and return value. free when done
  char * retval = NULL;
  file_search_func(file, "test", ",", &retval, 2, 48);
  printf("test = %s\n", retval);
  free(retval);

  fclose(file);
  file = NULL;

  return 0;
}

void file_search_func(FILE * file, const char * searchstr,
                      const char * delimiter, char ** pointer,
                      const int field, const int stringmax)
{
  // declare and initialize
  char * token;
  char * buffer = (char*)calloc(stringmax,sizeof(char));
  char * tmp = (char*)calloc(stringmax,sizeof(char));
  int counter = 0;
  int toggledone = 0;

  // confirm file opened successfully
  file_open_check(file, 'r', 999);

  // loop until find identifier in string. typically first field
  // tokenize once found
  while (fgets(buffer, stringmax, file)) {
    if (toggledone == 1) {
      break;
    }
    if (strstr(buffer, searchstr)) {
      token = strtok(buffer, delimiter);
      while (token) {
        // stops when field found of specified string
        // return and clean up
        if (counter == field) {
          strcpy(tmp, token);
          *pointer = tmp;
          toggledone = 1;
          break;
        } else {
        // continue to next token until found
          counter++;
          token = strtok(NULL, delimiter);
        }
      }
    }
  }
  free(buffer);
}

void file_open_check(const FILE *file, const char a, const int errcode)
{
  if (!file) {
    if (a == 'r') { // read error
      printf("error. unable to read file!");
    } else if (a == 'w') { // write error
      printf("error. unable to write file!");
    } else if (a == 't') { // temp file error
      printf("error. unable to create temp file!");
    }
    printf("\n\nfind the error %d in the man page.\n", errcode);
    printf("correct the problem before you try again.\n");

    // bail out
    exit(1);
  }
}

astrogeek · 03-04-2023, 04:33 PM

Could you please post an example of your input file, datafile.txt in you most recent code snippet.

That would help myself and others better understand your intent as well.

jmgibson1981 · 03-04-2023, 05:44 PM

Code:

jason,is,a,noob
i,love,my,house
my,dog,is,a,fool
test,me,out,please
cant,you,see,me

Ultimately I'm trying to build a simple banking type of thing. fields separated by comma in the file. I had googled for "small projects" to build my skillset. This was one of the first suggestions i found.

These file functions in particular i can see being useful in the future so I'm trying to make them for library purposes. Make them lgpl for anyone to use.