C, help with functions?

[Chris Gordon]

Hello, I was having some trouble attempting to code a program to parse an inf file, and output a file with a custom index of information from that inf. Currently I am having trouble with corrupted output from StrSplit from my source, and I was wondering if I could get some feedback on what im doing wrong?

Thanks!

Source:

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


char **FileReadArray(char File[]);
char *StrLeft(const char array[],int count);
char *StrRight(const char array[],int count);
char *StrTrimRight(char array[],int count);
char *StrTrimLeft(char array[],int count);
size_t StripWS(char *out, const char *str);
int CharInStr(char array[],char search,int rtl);
void FilterArray(char *array[]);
char *StrSplit(const char str[],int count);


int main(void){
    char Inf[]="Machine.ans.inf";
    printf("Reading...",Inf);
    char **data=FileReadArray(Inf);//Read File into Array.
    int i=(int)data[0];
    printf("Done\n");
    printf("Cleaning...\n");
    FilterArray(data);
    //printf("Failed\n");
    for (i;i>=1;i--){//Free Array
        free(data[i]);
    }
    free(data);
    return 0;
    }


/*
*/


void FilterArray(char *array[]){
    int i=(int)array[0];
    if (i<1) return 0;
    for (int j=1;j<=i;j++){


        //Strip Leading and Trailing Spaces.
        StripWS(array[j],array[j]);


        //Skip Comment lines, NULL or Space.
        if (array[j][0]==';') continue;
        if (array[j][0]==0x0) continue;
        if (array[j][0]==' ') continue;


        //Strip Comments from lines
        int cPos=CharInStr(array[j],';',1);
        if (cPos>0){
            array[j]=StrLeft(array[j],cPos-1);
        }


        //Strip Leading and Trailing Spaces.
        StripWS(array[j],array[j]);


        //Split Elements
        cPos=CharInStr(array[j],'=',1);
        if (cPos>0){
            array[j]=StrSplit(array[j],cPos);
         //   printf("%d|%s|\n\n",cPos,RightStr);
        }


        //char Right[strlen(array[j])-cPos];
    }


}
char *StrLeft(const char str[],int count){
    int iStrip=count-1;
    strncpy(str,str,iStrip);
    str[iStrip]='\0';
    return str;
}
char *StrRight(const char str[],int count){
    int iStrip=count;
    strncpy(str,str+iStrip,strlen(str)-1);
    str[strlen(str)+1]='\0';
    return str;
}


char *StrSplit(const char str[],int count){
    //char **split[2][4096];


    split[0]=StrLeft(str,count);
    split[1]=StrRight(str,count);
    printf("|%s|%s|\n",split[0],split[1]);
    free(split);
    return str;
}


size_t StripWS(char *out, const char *str){
  size_t len=strlen(out);
  const char *end;
  size_t out_size;


  // Trim leading space
  while(isspace(*str)) str++;


  if(*str == 0)  // All spaces?
  {
    *out = 0;
    return 1;
  }


  // Trim trailing space
  end = str + strlen(str) - 1;
  while(end > str && isspace(*end)) end--;
  end++;


  // Set output size to minimum of trimmed string length and buffer size minus 1
  out_size = (end - str) < len ? (end - str) : len;


  // Copy trimmed string and add null terminator
  memcpy(out, str, out_size);
  out[out_size] = 0;


  return out_size;
}


int CharInStr(char array[],char search,int rtl){
    int len=strlen(array);
    if (len<1) return 0;
    if (rtl==0) {
        for (int i=0;i<=len;i++){
            if (array[i]==search) return i+1;
        }
    } else if(rtl==1){
        for (int i=len;i>0;i--){
            if (array[i]==search) return i+1;
        }
    }
    return 0;
}


char **FileReadArray(char File[]){
    int lines_allocated=1;
    int max_line_len=4096;
    char **words=(char **)malloc(sizeof(char*)*lines_allocated);
    if (words==NULL){
        fprintf(stderr,"~!Error@InitArray, Cannot Allocate Initial Array.\n    Skipping \"%s\".",File);
        return 0;
    }
    FILE *fp = fopen(File,"r");
    if (fp == NULL){
        fprintf(stderr,"~!Error@fopen, Cannot open \"%s\", Skipping.\n",File);
        return 0;
    }
    int i;
    int j;
    for (i=1;1;i++){
        if (i >= lines_allocated){ /* Have we gone over our line allocation? */
            int new_size;
            new_size=lines_allocated+1; /* Add to allocation and re-allocate */
            words=(char **)realloc(words,sizeof(char*)*new_size);
            if (words==NULL){
                fprintf(stderr,"~!Error@ReAllocate, Failed to allocate new array. Skipping \"%s\".\n",File);
                return 0;
            }
            lines_allocated=new_size;
        }
        words[i]=malloc(max_line_len); /* Allocate space for the next line */
        if (words[i]==NULL){
            fprintf(stderr,"~!Error@ReAllocate, Failed to allow new line. Skipping \"%s\".\n",File);
            return 0;
            }
        if (fgets(words[i],max_line_len-1,fp)==NULL) break;
        for (j=strlen(words[i])-1;j>=0 && (words[i][j]=='\n' || words[i][j]=='\r');j--);//Strip CR+LF from end of line.
        words[i][j+1]='\0';
    }
    (int)words[0]=i;
    fclose(fp);
    return words;
}
    //ArrayInfGetSections,
    //ArrayInfGetElement, Parse Array, Return Element if exists
    //CleanString, Strip Quotes, Comments, etc.

[Salem]

There is a fundamental problem with StrLeft, StrRight and StripWS
Neither strcpy, strncpy or memcpy are defined for overlapping memory.

That is, you can't rely on something like this always working.

char test[] = "hello world";
strcpy(test,test+2);

In all cases, the best thing to do is supply another parameter which points to a different area of memory.
char *StrLeft(char *out, const char str[],int count)
If the caller wants to copy the temporary buffer over the original source, then it's free to do so knowing that it's safe to do so.

Fixing split would be something like this.
Though perhaps you want split to be an output parameter, rather than local.

char *StrSplit(const char str[],int count){
    char *split[2];
 
    split[0]=StrLeft(str,count);
    split[1]=StrRight(str,count);
    printf("|%s|%s|\n",split[0],split[1]);
    // you didn't malloc, so don't free(split);
    return str;
}

> char **words=(char **)malloc(sizeof(char*)*lines_allocated);
Don't cast the result of malloc in a C program (see the FAQ).
If you've made the mistake of naming your source file as a .cpp file, you should correct this.

> new_size=lines_allocated+1; /* Add to allocation and re-allocate */
You're going to be calling realloc for every line you read, and this will be expensive.
Doing something like
new_size=lines_allocated * 2; /* Add to allocation and re-allocate */
If necessary, you can do one final realloc at the end to make the array of pointers the correct length.

> words=(char **)realloc(words,sizeof(char*)*new_size);
If realloc fails, then you've leaked all your memory. You should do this

void *temp = realloc(words,sizeof(char*)*new_size);
if ( temp ) {
  words = temp;
} else {
  // clean up words, or whatever
}

> if (fgets(words[i],max_line_len-1,fp)==NULL) break;
> for (j=strlen(words[i])-1;j>=0 && (words[i][j]=='\n' || words[i][j]=='\r');j--);//Strip CR+LF from end of line.
> words[i][j+1]='\0';
1. You don't need to subtract 1 from the buffer size passed to fgets
2. You won't normally see a \r in the buffer even on systems where \r\n is the line terminator, when the file is opened in text mode.


> (int)words[0]=i;
What a truly awful abuse of pointers.

Do something like this, and finish with *numLinesRead = i;
char **FileReadArray(char File[], int *numLinesRead)
When you've fixed that, then you need to pass numLinesRead as a parameter to all your other functions, and you can start reading data[] from index 0, not index 1.

[Chris Gordon]

Thank you for the tips!

I'm going to completely rewrite this code with those tips in mind.

Instead of having separate functions to format each line, im going to try reading the inf, and only alloc array elements as needed for the data im trying to extract.

[Chris Gordon]

There is a fundamental problem with StrLeft, StrRight and StripWS
Neither strcpy, strncpy or memcpy are defined for overlapping memory.

That is, you can't rely on something like this always working.

char test[] = "hello world";
strcpy(test,test+2);

In all cases, the best thing to do is supply another parameter which points to a different area of memory.
char *StrLeft(char *out, const char str[],int count)
If the caller wants to copy the temporary buffer over the original source, then it's free to do so knowing that it's safe to do so.

Fixing split would be something like this.
Though perhaps you want split to be an output parameter, rather than local.

char *StrSplit(const char str[],int count){
    char *split[2];
 
    split[0]=StrLeft(str,count);
    split[1]=StrRight(str,count);
    printf("|%s|%s|\n",split[0],split[1]);
    // you didn't malloc, so don't free(split);
    return str;
}

> char **words=(char **)malloc(sizeof(char*)*lines_allocated);
Don't cast the result of malloc in a C program (see the FAQ).
If you've made the mistake of naming your source file as a .cpp file, you should correct this.

> new_size=lines_allocated+1; /* Add to allocation and re-allocate */
You're going to be calling realloc for every line you read, and this will be expensive.
Doing something like
new_size=lines_allocated * 2; /* Add to allocation and re-allocate */
If necessary, you can do one final realloc at the end to make the array of pointers the correct length.

> words=(char **)realloc(words,sizeof(char*)*new_size);
If realloc fails, then you've leaked all your memory. You should do this

void *temp = realloc(words,sizeof(char*)*new_size);
if ( temp ) {
  words = temp;
} else {
  // clean up words, or whatever
}

> if (fgets(words[i],max_line_len-1,fp)==NULL) break;
> for (j=strlen(words[i])-1;j>=0 && (words[i][j]=='\n' || words[i][j]=='\r');j--);//Strip CR+LF from end of line.
> words[i][j+1]='\0';
1. You don't need to subtract 1 from the buffer size passed to fgets
2. You won't normally see a \r in the buffer even on systems where \r\n is the line terminator, when the file is opened in text mode.


> (int)words[0]=i;
What a truly awful abuse of pointers.

Do something like this, and finish with *numLinesRead = i;
char **FileReadArray(char File[], int *numLinesRead)
When you've fixed that, then you need to pass numLinesRead as a parameter to all your other functions, and you can start reading data[] from index 0, not index 1.

"Don't cast the result of malloc in a C program (see the FAQ)."

I keep getting the warning: assignment makes pointer from integer without a cast.

[laserlight]

I keep getting the warning: assignment makes pointer from integer without a cast.

You probably forgot to #include <stdlib.h> for the declaration of malloc. Without such a declaration in scope, for historical reasons the compiler may assume that malloc is declared as returning an int, and hence warn you about it. The cast silences this warning, but the correct approach is to fix it by providing the declaration, in this case by including the relevant header.

[Chris Gordon]

ah, ok

[Chris Gordon]

Working on another part of the code, scanning for inf files in C:\.
it gets to 2645 and crashes, sometimes it locks up on 19 causing one of my cores to have max load.

                        
                        int newSize=filecnt*2;
                        realloc(aFiles,sizeof(MAX_PATH)*newSize);
                        if (aFiles==NULL) return 7;
                        aFiles[iFiles]=malloc(MAX_PATH);//Crashes here.
                        if (aFiles[iFiles]==NULL) return 8;
                        strcpy(aFiles[iFiles],currFile);
                        if (aFiles[iFiles]==NULL) return 9;
                        printf("%d|%s\n",iFiles,aFiles[iFiles]);
                        iFiles++;
                        filecnt=newSize;

Full code:

#include <windows.h>
#include <direct.h>
#include <stdio.h>
#include <limits.h>
#include <stdlib.h>


WIN32_FIND_DATA *fd;
int iFiles=0;


int FixPath(const char *inPath, char *outPath){
  int n=0;
  strcpy(outPath,inPath);
  while(inPath[n]) n++;
  if(inPath[n-1] != '\\'){
    strcat(outPath,"\\");
    return 1;
  }
  return 0;
}


int CharInStr(char array[],char search,int rtl){
    int len=strlen(array);
    if (len<1) return 0;
    if (rtl==0) {
        for (int i=0;i<=len;i++){
            if (array[i]==search) return i+1;
        }
    } else if(rtl==1){
        for (int i=len;i>0;i--){
            if (array[i]==search) return i+1;
        }
    }
    return 0;
}


int GetFiles(char *aFiles[],const char *_path, const char *type){
    HANDLE fh;
    int filecnt=1;
    char path[MAX_PATH];
    char tmppath[MAX_PATH];
    char currpath[MAX_PATH];
    strcpy(currpath,_path);
    fd=malloc(sizeof(WIN32_FIND_DATA));
    FixPath(_path,path);
    strcat(path,"*");
    fh=FindFirstFile((LPCSTR) path,fd);
    if(fh!=INVALID_HANDLE_VALUE){
        do{
            if(fd->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY){
                if((0 != strcmp(fd->cFileName,".")) && (0 != strcmp(fd->cFileName,".."))){
                    FixPath(_path,tmppath);
                    strcat(tmppath,fd->cFileName);
                    GetFiles(aFiles,tmppath,type);
                }
            }else{
                char currFile[MAX_PATH];
                strcpy(currFile,_path);
                strcat(currFile,"\\");
                strcat(currFile,fd->cFileName);
                int iExt=CharInStr(currFile,'.',1);
                char cExt[strlen(currFile)-iExt];
                memcpy(cExt,&currFile[iExt],strlen(currFile)-iExt);
                cExt[strlen(currFile)-iExt]='\0';
                if (!strcmp(cExt,type)){
                    if (iFiles==0){
                        strcpy(aFiles[iFiles],currFile);
                        printf("%d|%s\n",iFiles,aFiles[iFiles]);
                        iFiles++;
                    }else if (iFiles>0){
                        int newSize=filecnt+1;
                        realloc(aFiles,sizeof(MAX_PATH)*newSize);
                        if (aFiles==NULL) return 7;
                        aFiles[iFiles]=malloc(MAX_PATH);
                        if (aFiles[iFiles]==NULL) return 8;
                        strcpy(aFiles[iFiles],currFile);
                        if (aFiles[iFiles]==NULL) return 9;
                        printf("%d|%s\n",iFiles,aFiles[iFiles]);
                        iFiles++;
                        filecnt=newSize;
                    }
                }
            }
        }while(FindNextFile(fh,fd));
    }
    FindClose(fh);
    return 1;
}


char cPath[PATH_MAX];
WIN32_FIND_DATA *tFind;


int main(int argc, char *argv[])
{
    //Handle Arguments
    if (argc==1) {
        _getcwd(cPath,sizeof(cPath));
    } else if (argc==2) {
        strncpy(cPath,argv[1],strlen(argv[1]));
    }
    char **aFiles=malloc(sizeof(char*));
    aFiles[0]=malloc(MAX_PATH);
    GetFiles(aFiles,cPath,"inf");
    return 0;
}

Thanks

[laserlight]

Examine this snippet:

int newSize=filecnt*2;
realloc(aFiles,sizeof(MAX_PATH)*newSize);
if (aFiles==NULL) return 7;

You call realloc to increase the allocated memory, but it is not clear why you multiply newSize by sizeof(MAX_PATH). Furthermore, you did not assign the pointer returned by realloc to a pointer variable. Not only that, but why return 7? That is a magic number. Rather, you should have written somewhere near the top of your code:

enum ErrorCode {
    REALLOCATION_FAILURE = 7,
};

Then to replace the code snippet above:

int newSize = filecnt * 2;
char **temp = realloc(aFiles, sizeof(aFiles[0]) * newSize);
if (temp) {
    aFiles = temp;
} else {
    return REALLOCATION_FAILURE;
}

[Chris Gordon]

I updated source, but still crashes at random, some times at 2646th realloc, sometimes around 400, and even on 20th.

[Salem]

Post the latest code.

[Hodor]

Edit: Wrong info

[Chris Gordon]

Full code:

#include <windows.h>
#include <direct.h>
#include <stdio.h>
#include <limits.h>
#include <stdlib.h>




WIN32_FIND_DATA *fd;
int iFiles=0;




int FixPath(const char *inPath, char *outPath){
  int n=0;
  strcpy(outPath,inPath);
  while(inPath[n]) n++;
  if(inPath[n-1] != '\\'){
    strcat(outPath,"\\");
    return 1;
  }
  return 0;
}




int CharInStr(char array[],char search,int rtl){
    int len=strlen(array);
    if (len<1) return 0;
    if (rtl==0) {
        for (int i=0;i<=len;i++){
            if (array[i]==search) return i+1;
        }
    } else if(rtl==1){
        for (int i=len;i>0;i--){
            if (array[i]==search) return i+1;
        }
    }
    return 0;
}




int GetFiles(char *aFiles[],const char *_path, const char *type){
    HANDLE fh;
    int filecnt=1;
    char path[MAX_PATH];
    char tmppath[MAX_PATH];
    char currpath[MAX_PATH];
    strcpy(currpath,_path);
    fd=malloc(sizeof(WIN32_FIND_DATA));
    FixPath(_path,path);
    strcat(path,"*");
    fh=FindFirstFile((LPCSTR) path,fd);
    if(fh!=INVALID_HANDLE_VALUE){
        do{
            if(fd->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY){
                if((0 != strcmp(fd->cFileName,".")) && (0 != strcmp(fd->cFileName,".."))){
                    FixPath(_path,tmppath);
                    strcat(tmppath,fd->cFileName);
                    GetFiles(aFiles,tmppath,type);
                }
            }else{
                char currFile[MAX_PATH];
                strcpy(currFile,_path);
                strcat(currFile,"\\");
                strcat(currFile,fd->cFileName);
                int iExt=CharInStr(currFile,'.',1);
                char cExt[strlen(currFile)-iExt];
                memcpy(cExt,&currFile[iExt],strlen(currFile)-iExt);
                cExt[strlen(currFile)-iExt]='\0';
                if (!strcmp(cExt,type)){
                    if (iFiles==0){
                        strcpy(aFiles[iFiles],currFile);
                        printf("%d|%s\n",iFiles,aFiles[iFiles]);
                        iFiles++;
                    }else if (iFiles>0){
                        int newSize=filecnt*2;
                        realloc(aFiles,sizeof(MAX_PATH)*newSize);
                        if (aFiles==NULL) return 7;
                        aFiles[iFiles]=malloc(MAX_PATH);
                        if (aFiles[iFiles]==NULL) return 8;
                        strcpy(aFiles[iFiles],currFile);
                        if (aFiles[iFiles]==NULL) return 9;
                        printf("%d|%s\n",iFiles,aFiles[iFiles]);
                        iFiles++;
                        filecnt=newSize;
                    }
                }
            }
        }while(FindNextFile(fh,fd));
    }
    FindClose(fh);
    return 1;
}




char cPath[PATH_MAX];
WIN32_FIND_DATA *tFind;




int main(int argc, char *argv[])
{
    //Handle Arguments
    if (argc==1) {
        _getcwd(cPath,sizeof(cPath));
    } else if (argc==2) {
        strncpy(cPath,argv[1],strlen(argv[1]));
    }
    char **aFiles=malloc(sizeof(MAX_PATH));
    aFiles[0]=malloc(MAX_PATH);
    GetFiles(aFiles,cPath,"inf");
    for (int i=0;i<iFiles;i++){
        printf("%d|%s\n",i,aFiles[i]);
        //ParseINF()
        free(aFiles[i]);
    }
    return 0;
}

If I comment out lines 74-76, 84, and change 106 to:

char **aFiles=malloc(sizeof(MAX_PATH)*(1024*32));

it works great, however I don't see the need to allocate that much.

I thought to call GetFiles() first with a parameter to scan only, to just get the number of inf files, then alloc aFiles, then populate file path/names, then start parsing. but that approach does not seem to efficient.

[Chris Gordon]

Anyone?

[Salem]

> char **aFiles=malloc(sizeof(MAX_PATH));
What do you think sizeof(MAX_PATH) is going to evaluate to?

> realloc(aFiles,sizeof(MAX_PATH)*newSize);
> if (aFiles==NULL) return 7;
You didn't read laserlight's post either.
Realloc returns a result, which you're just ignoring.

Another problem, if realloc does move the memory, then you need a way of returning that changed pointer back to main.

> fd=malloc(sizeof(WIN32_FIND_DATA));
There is no free(fd) in the code.