Possible buffer overflow dangers in C log parsing program

After hours of working, I've finally finished my first C log parsing program! (previously was a bash script, now it is C).

Although I think I've gotten most everything, I was just wondering if you saw any possible buffer/memory overflow dangers in the coding?

```
#include
#include
#include
#include

#ifndef max
#define max(a, b) ((a)>(b))? (a) : (b)
#endif

long GetFileSize(FILE *fp){
long fsize = 0;

fseek(fp,0,SEEK_END);
fsize = ftell(fp);
fseek(fp,0,SEEK_SET);//reset stream position!!

return fsize;
}
char lastline(char filepath){
FILE *fp;
char buff[4096+1];
int size,i;
long fsize;
if(NULL==(fp=fopen(filepath, "r"))){
printf("You have not died recently enough for any information to be recorded.");
return NULL;
}
fsize= -1L*GetFileSize(fp);
if(size=fseek(fp, max(fsize, -4096L), SEEK_END)){
perror("cannot seek");
exit(0);
}
size=fread(buff, sizeof(char), 4096, fp);
fclose(fp);
buff[size] = '\0';
i=size-1;
if(buff[i]=='\n'){
buff[i] = '\0';
}
while(i >=0 && buff[i] != '\n')
--i;
++i;
return strdup(&buff[i]);
}

int main(int argc, char argv[], char envp[]){
typedef char * string;
char *last;
char *name;
char field_x[128];
char field_y[128];
char field_z[128];
char field_world[128];
char field_cause[128];
char field_killer[128];
name = getenv("MCEXEC_PLAYERNAME");
char *filename;
char *p;
char *ispvp;
int i;
char *f;
char output[200] = {0x00};
int index = 0;
char field_year[128];
char field_month[128];
char field_mix[128];
char field_mix2[128];
char field_day[128];
char field_hour[128];
char field_minute[128];
char field_seconds[128];
char *hummonth;
int dmon;
int dy;
char *cause_string;
int x;
char *deathtype;
string dtypes[15] = { "unknown", "water", "fire", "explosion", "lava", "fall",

There are several things to improve in the code before looking for buffer overflows:

• Don't duplicate constant strings. E.g. sizeof("You were killed by §f\n") and then snprintf(..., "You were killed by §f\n", ...))

• Avoid raw constants (and duplicated) in the code. E.g. char buff[4096+1]; ... fread(buff, sizeof(char), 4096, fp); In this case you can define a constant/macro for 4096 or just use sizeof(buff) in fread(buff, sizeof(char), sizeof(buff)-1, fp);.

-
In the code:

while(i >=0 && buff[i] != '\n')
    --i;
    ++i;

The ++i; has a invalid indentation which can confuse the reader.

-
Your macro max should have some parenthesis to enclose the whole result:

#define max(a, b) ( ((a)>(b))? (a) : (b) )

Without those parenthesis a call like (max(2,1)+3) will give you 2 (instead of 5).

• You've defined string type like typedef char string but you're using the string type like const char.

• You're calling several times snprintf function with an invalid 2nd argument (which is the same as calling sprintf directly). If you are creating a buffer with a specific size in malloc, remember to use that size in snprintf().

-
In

dmon = atoi(field_month);
hummonth = realm[dmon-1];

The atoi is generally deprecated, because it's not required to set errno -- when it returns a zero, you won't know why. You should use strtol and ensure that dmon has sensible value. If dmon is zero, then errno will indicate whether it was really zero, or was there an error. In this case simply checking dmon to be between 1 and 12 inclusive will do the trick, as the zero value is not allowed.

There are more things, but anyway, generally I would recommend you to create tiny functions to do specific tasks: instead of having mallocs/snprints/scanfs all mixed together, write functions with minimal objectives which you can ensure that they don't contain bugs/memory buffer overflow. Then you are safe to use them in your main() function.

StackExchange Code Review Q#11886, answer score: 6