Strange segfault on linux only

[ninja9578]

I'm receiving a very strange segfault on line 94

char buffer[256];

in the traverse method below, the first time it runs. This is very strange and I have no idea what is causing it. I've debugged the program on XCode and see no problems, and I'm starting to think that it's a gcc bug.

Whatever it is is even causing Valgrind to segfault. Valgrind picks up several jumps based on uninitialized values within strcat. Is this a gcc bug, or does someone see a problem with my code?

#include <stdio.h>
#include <dirent.h>
#include <sys/stat.h>
#include <string.h>
#include <stdbool.h>
#include <stdlib.h>

char * findTab(char * str){
    bool quote = false;
    for (;*str;++str){
	   switch(*str){
		  case '\\':
			 ++str;
			 if (!(*str)) return NULL;
			 break;
		  case '\"':
			 quote = !quote;
			 break;
		  case '\t':
			 if (!quote) return str;
			 break;
	   }
    }
    return NULL;
}

static bool tryfile(char * filename){
    printf("Working on %s... ", filename);
    
    char * ext = filename + strlen(filename) - 4;
    if (strcmp(ext, ".php")){
	   printf("skipping\n");
	   return false;  //not a PHP file
    }
    
    struct stat stbuf;
    stat(filename, &stbuf);
    
    FILE * fp = fopen(filename, "r");
    if (!fp){
	   printf("failed to open\n");
	   return false;
    }
    
    size_t characters = stbuf.st_size * 2;
    char * contents = (char*)malloc(characters * sizeof(char));
    size_t len = fread(contents, 1, stbuf.st_size, fp);
    fclose(fp);
    
    if (len != stbuf.st_size){
	   printf("load failed\n");
	   free(contents);
	   return false;
    }
    
    contents[len] = '\0';
    char * tab = findTab(contents);
    int tabs = 0;
    while (tab){
	   char * buffer = (char*)malloc(characters * sizeof(char));
	   memcpy(buffer, contents, ((long)tab - (long)contents) * sizeof(char));
	   strcat(buffer, "    ");
	   strcat(buffer, tab + 1);
	   strcpy(contents, buffer);
	   free(buffer);
	   if (strlen(contents) > characters - 10){
		  characters *= 2;
		  contents = (char*)realloc(contents, characters);
	   }
	   ++tabs;
	   tab = findTab(contents);
    }
    
    fp = fopen(filename, "w");
    if (!fp){
	   printf("failed to open for output\n");
	   free(contents);
	   return false;
    }
    
    size_t bytes = strlen(contents) * sizeof(char);
    size_t check = fwrite(contents, 1, bytes, fp);
    fclose(fp);
    free(contents);
    if (bytes != check){
	   printf("failed to write new file\n", tabs);
	   return false;
    }
    printf("done, replaced %d tabs\n", tabs);
    return true;
}

static bool traverse(char * name, bool recurse){
    char buffer[256];
    strcpy(buffer, name);
    strcat(buffer, "/");
    DIR * dir = opendir(name);
    if (dir){
	   struct dirent * dp;
	   if (!(dp = readdir(dir))) return false; //.
	   if (!(dp = readdir(dir))) return false; //..
	   while (dp = readdir(dir)){
		  if (dp -> d_name[0] != '.'){  //ignore hidden files and folders
			 strcpy(buffer, name);
			 strcat(buffer, "/");
			 strcat(buffer, dp -> d_name);
			 if ((!recurse) || (!traverse(buffer, recurse))){
				tryfile(buffer);
			 }
		  }
	   }
	   return true;
    }
    //not a directory
    return false;	
}

int main(int argc, char ** argv){
    printf("Checking parameter count\n");
    //check that the dir param is there
    if ((argc != 2) && (argc != 3)){
	   printf("You have to give this program a directory as an argument\n");
	   return 0;
    }
    
    printf("Checking for recursive param\n");
    bool recurse = false;
    if (argc == 3){
	   printf("Checking recursive param is correct\n");
	   if (strcmp(argv[2], "-r")){
		  printf("The correct parameters are %s file/dir [-r]\n", argv[0]);
		  return 0;
	   }
	   recurse = true;
    }
    
    printf("Starting %s\n", argv[1]);
    if (!traverse(argv[1], recurse)){
	   if (!tryfile(argv[1])){
		  printf("%s doesnt appear to exist or is not a directory\n", argv[1]);
		  return 0;
	   }
    }
    
    return 0;
}

[Lindley]

Well, a good start would be avoiding the potential buffer overflow attack by using strncat() and strncpy() rather than their unbounded equivalents. Don't forget to set buffer[255] = 0 afterwords, just in case.

[ninja9578]

It has to be one of the std libraries. I tried it on another box, same OS, same compiler, and it worked fine. I added the bound checking versions and no difference, it still segfaults when trying to create the buffer.

[Paul McKenzie]

It has to be one of the std libraries. I tried it on another box, same OS, same compiler, and it worked fine.

That doesn't prove anything really, except you have a lot of compilers to test to see which ones don't expose the bug.

Never blame the compiler or the library unless there is bonafide proof of that being the issue. I have seen programs run on 100 machines and bomb out on machine 101 because of a bug in the program. In your code, if you're off by 1 byte anywhere, anything can happen. You are doing a lot of raw character buffer manipulation with little to no check for boundary conditions.

First, what is the command line you used? What data file did you use? We don't know, since your code is dependent on those things to run.

Regards,

Paul McKenzie

[monarch_dodra]

I'd try trimming it to the smallest possible program that produces this behaviour and take it from there. Does this also segfault?

#include <stdio.h>
#include <dirent.h>
#include <sys/stat.h>
#include <string.h>
#include <stdbool.h>
#include <stdlib.h>

static bool traverse(char * name, bool recurse){
    char buffer[256];
    strcpy(buffer, name);
    strcat(buffer, "/");
    DIR * dir = opendir(name);
    if (dir){
	   struct dirent * dp;
	   if (!(dp = readdir(dir))) return false; //.
	   if (!(dp = readdir(dir))) return false; //..
	   while (dp = readdir(dir)){
		  if (dp -> d_name[0] != '.'){  //ignore hidden files and folders
			 strcpy(buffer, name);
			 strcat(buffer, "/");
			 strcat(buffer, dp -> d_name);
		  }
	   }
	   return true;
    }
    //not a directory
    return false;	
}

int main(int argc, char ** argv){
    traverse(argv[1], false);  
    return 0;
}

Does it also crash in debug? Do you have access to the assembly? What you have is valid C, so I can only think of 2 reasons for such a crash:
1- Stack Corruption
2- Stack Overflow

[S_M_A]

Frankly, isn't this an quite easy case to find out if there's a buffer overflow error or not?
Declare char buffer[257] = {0};
Call traverse using the same params that case the segfault and check that buffer[256] is 0 when returning. If not there's a buffer overflow.

[Lindley]

The point was not so much that a buffer overflow was causing the problem, but that code which admits the possibility of one is undesirable.

[S_M_A]

I totally agree on that Lindley but I thought that proving that it really happens could have the benefit of showing that it's not gcc a bug.

After all we've all been there, blaming the compiler and in the end found out that we're the one that has made something stupid...

[Moschops]

I haven't definitely seen a compiler bug for a decade. It would have been 2001, using the compiler that came with VS6.

[S_M_A]

Yeah, last time I saw one was also about 10 years ago in an compiler for an embedded system so they are definitely getting better.

[Paul McKenzie]

char buffer[256]
//...
strcpy(buffer, name);
strcat(buffer, "/");
strcat(buffer, dp -> d_name);

Let's see:

buffer is 256 characters, so we are copying a name that is potentially 255 characters in length. Then we're concatenating a "/" onto that. Then we're concatenating another name "dp->d_name" that is potentially 255 characters. That is 512 characters being stuffed in a buffer that can only hold 256 characters.

So I could easily make this program have undefined behaviour by having very long file and directory names.

Regards,

Paul McKenzie

[Lindley]

Yeah, last time I saw one was also about 10 years ago in an compiler for an embedded system so they are definitely getting better.

Lucky me, I found (and reported) a bug which repeatably crashed gcc less than a year ago.