0

I am trying to remove the last comma from a record. I use strrchr() to find the last occurrence of ',' in the record and set it to the null termination. For some reason it is not finding the last occurrence of the comma and giving a "segmentation fault 11" error.

void buildAssemblyRecord(char asmRecord[], const char* data)
{
char* record = asmRecord;
record += sprintf(record, "dc.b\t");

int i = 0;
for(i = 0; i < strlen(data); i++)
{
    record += sprintf(record, "$%.2X, ", data[i]);
}

//Remove trailing comma
char* whereComma = strrchr(record, ',');
if(whereComma != NULL)
{
    *whereComma = '\0';
}
}

Theoretically this should work perfectly, as I use this method all the time with regular old strchr to remove new line characters from fgets input.

Could anyone let me know whats going on?

Kyle Jensen
  • 419
  • 9
  • 27
  • 1
    Did you check for NULL? – Sourav Ghosh Feb 26 '16 at 17:37
  • I do have this but all its doing is stopping the error. I want to know why it is not recognizing the comma.. – Kyle Jensen Feb 26 '16 at 17:39
  • 1
    ***Show your input***. How can we diagnose a problem on "mystery" input? – abelenky Feb 26 '16 at 17:39
  • I have shown the whole function – Kyle Jensen Feb 26 '16 at 17:40
  • 3
    Looks like `record` after all arithmetic would point at the last null terminator. Therefore, there's no comma in the remaining string it points to and strrchr returns NULL. – Joachim Isaksson Feb 26 '16 at 17:42
  • `strrchr(ptr, char)` will not find a character *before* the `ptr`. Don't just add the return value of `sprintf()` to `record`. – EOF Feb 26 '16 at 17:43
  • Yeah this is the way a post on stack overflow told me to build strings, didn't think of it that way. How can I do this correctly then? – Kyle Jensen Feb 26 '16 at 17:45
  • I see now that you *do* check for a null pointer, but then you should not have the crash. So either you don't have the crash where you think it is, or you don't actually do the null pointer check. If you *do* have the null pointer check then you need to run in a debugger to catch the error "in action" and see where in your code it is. – Some programmer dude Feb 26 '16 at 17:48
  • I removed the NULL check in the first post and afterwards realized that it wasn't a problem with the segmentation fault as it was with pointer arithmetic – Kyle Jensen Feb 26 '16 at 17:50

1 Answers1

3

If you read e.g. this sprintf (and family) reference you will see that it returns the length of the string it writes.

When you do record += sprintf(...) you make record point beyond the the newly printed string. Which is good for your loop. But then you use record directly in the strrchr call, and strrchr can't find the character you look for and will return NULL which you do not check for. So when you dereference whereComma you will dereference a null pointer and have undefined behavior and your crash.

You need to reset the pointer after the loop:

record = asmRecord;
Some programmer dude
  • 400,186
  • 35
  • 402
  • 621