Print statement changing char array output

Question

I'm trying to convert some text (character by character) to its binary representation. For some reason the print statement printf("Hold is %d or %c: ", hold, hold); is changing the output of my function and I have no idea how to explain it. Any help would be greatly appreciated. The test file is just a text file with Hello, World! inside of it.

With it:

Hold is 72 or H: 01001000
Hold is 101 or e: 01100101
Hold is 108 or l: 01101100
Hold is 108 or l: 01101100
Hold is 111 or o: 01101111
Hold is 44 or ,: 00101100
Hold is 32 or  : 00100000
Hold is 87 or W: 01010111
Hold is 111 or o: 01101111
Hold is 114 or r: 01110010
Hold is 108 or l: 01101100
Hold is 100 or d: 01100100
Hold is 33 or !: 00100001

Without it:

1000 �
0101 �
1100 �
1100 �
1111 �
1100 �
0000 �
0111 �
1111 �
0010 �
1100 �
0100 �
0001 �

Code

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

void decimal_to_binary(unsigned long num, FILE *out) {    
    int i = 255, a = 0;
    char binarr[255];
    for (i = 0; i < 255; i++) { binarr[i] = '0'; }
    if (num != 0) {
        while (num != 0) {
            if (num % 2 == 0) {
                binarr[i] = '0';
                i--;
            } else {
                binarr[i] = '1';
                i--;
            }
            num /= 2;
        }
    } else {
        fprintf(out, "00000000");
    }
    fprintf(out, "%s ", binarr + strlen(binarr) - 8);
    printf("%s\n", binarr + strlen(binarr) - 8);
    memset(binarr, 0, sizeof(binarr));    
}

int main(int argc, char *argv[]) {
    int hold;
    FILE *in = fopen(argv[1], "r");
    FILE *out = fopen(argv[2], "w+");

    while (!feof(in)) {
        hold = fgetc(in);
        if (hold > 0 && hold != 10){
            printf("Hold is %d or %c: ", hold, hold);
            decimal_to_binary(hold, out);
        }
    }
    fclose(in);
    fclose(out);
    return 0;
}

Answer 1

Your decimal_to_binary function is incorrect:

• you index beyond the end of the binarr array.
• you do not null terminate this array to pass it to printf.

Here is a simpler and corrected version:

void decimal_to_binary(unsigned long num, FILE *out) {
    int i = 256, a = 0;
    char binarr[257];
    memset(binarr, '0', sizeof(binarr) - 1);
    binarr[i] = '\0';
    while (num != 0) {
        --i;
        if (num % 2) {
            binarr[i] = '1';
        }
        num /= 2;
    }
    if (i > 256 - 8) // print at least 8 bits
        i = 256 - 8;
    fprintf(out, "%s ", binarr + i);
    printf("%s\n", binarr + i);
}

Your function main has problems too:

• you test for end of file with feof(in). This is incorrect, you should instead check if hold is EOF.
• hard coding the value of '\n' as 10 is bad practice.

Here is a correct version:

int main(int argc, char *argv[]) {
    int hold;
    FILE *in = fopen(argv[1], "r");
    FILE *out = fopen(argv[2], "w+");

    while ((hold = fgetc(in)) != EOF) {
        if (hold != '\n') {
            printf("Hold is %d or %c: ", hold, hold);
            decimal_to_binary(hold, out);
        }
    }
    fclose(in);
    fclose(out);
    return 0;
}

Answer 2

Your program has undefined behavior for couple of reasons.

1. You don't have a null terminated string. Calling strlen on such a string is cause for undefined behavior.
2. You are modifying binarr using an out of bounds index. That is also cause for undefined behavior.

I have my annotations to your function decimal_to_binary that point out where those errors are.

void decimal_to_binary(unsigned long num, FILE *out){

    int i = 255, a = 0;
    char binarr[255];
    for (i=0; i<255; i++){ binarr[i] = '0'; }

    // All the elements of binarr are set to '0'.
    // It's not a null terminated string.

    if (num != 0) {
        while (num!=0){

           // The value of i is 255 when this loop is 
           // entered the first time.
           // Setting the value of binarr[255] is cause for
           // undefined behavior.

            if (num%2 == 0){

                binarr[i] = '0';
                i--;
            }
            else { binarr[i] = '1'; i--; }
            num /= 2;
        }
    } else { fprintf(out, "00000000"); }
    fprintf(out, "%s ", binarr + strlen(binarr) - 8);
    printf("%s\n", binarr + strlen(binarr) - 8);
    memset(binarr, 0, sizeof(binarr));
}

The fixes are simple.

1. Terminate string with the null character.

   for (i=0; i<255; i++){ binarr[i] = '0'; }
   i--;
   binarr[i] = '\0';
   

2. Use the right index when modifying binarr in the while loop.

   while (num!=0){
   
       // Decrement the index before you assign to the next element.
       // When the loop is entered the first time, i = 254, which
       // is used to null terminate binarray.
       // The next '1' or '0' needs to be placed at i = 253.
       i--;
   
       if (num%2 == 0){
   
           binarr[i] = '0';
       }
       else {
          binarr[i] = '1';
       }
       num /= 2;
   }
   

Answer 3

I decreased the extremely large array, made sure to terminate the string with a null character, zeroed the array, then printed it using fprintf. This solved the issue.

void decimal_to_binary(unsigned long num, FILE *out){

    int i = 7, a = 0;
    char binarr[9];
    binarr[8]='\0';
    for (a=7; a>=0; a--){ binarr[a] = '0'; }
    if (num != 0) {
        while (num!=0){
            if (num%2 == 0){
                binarr[i] = '0';
                i--;
            }
            else { binarr[i] = '1'; i--; }
            num /= 2;
        }
    } else { fprintf(out, "00000000"); }
    fprintf(out, "%s ", binarr);
    memset(binarr, 0, sizeof(binarr));
}