Thread: character array filling with junk

  1. #1
    Registered User
    Join Date
    Jan 2014
    Posts
    15

    character array filling with junk

    This is a tough one for me:

    A long time ago, I made myself a function that allows me to read input from user, validate it in a limited way, and put it into an array. The function:

    Code:
    int cInputString(int iMaxChar, char *cArr, int iValidate)
    {
    /* This is a character input function to limit the length of characters
       inputted by the user.  This is necessary because without using such a 
       function, user could overrun the input stack and cause errors.  It'll 
       accept single-character inputs until it hits the number of characters 
       requested (iMaxChar) or until the user hits Enter. */
    /* iValidate determines whether to check for input.  Data is checked based on
       the following table:
       0: do not check data
       1: verify all data is alpha (lower and upper-case letters)
       2: verify all data is numeric */
    /* int return is the length of the characters inputted */
      fflush(stdin);
      char cC;
      int iCount = 0;
      do
        {
          cC = getch();
          printf ("%c",cC);
          switch (iValidate)
          {
            case 1:
            {
              if((isalpha(cC))||cC==' '||cC==0xd)
              {
                cArr[iCount] = cC;
                ++iCount;
                break;
              }
              else
              {
                printf("\nInvalid Entry, use only letters.  Please try again: ");
                break;
              }
            }
            case 2: 
              {   
              if (atol(&cC)==0 && cC != '0' && cC != 0xd)
                {
                  printf("\nInvalid Entry, use only numbers.  Please try again: ");
                  break;
                }
              else if (cC == 0xd) break;
              else
                {
                  cArr[iCount] = cC;
                  ++iCount;
                  break;
                }
              }
            default:
              {                 
                cArr[iCount] = cC;
                ++iCount;
                break;
              }
          }
        }
      while (iCount < iMaxChar && cC != 0xd);
      printf ("\n");
      fflush(stdin);
      return iCount;
    }
    It works perfectly to get user data:
    Code:
      char cInput[7];
      cInputString(6,cInput,2);
      cInput[7]='\n';
      printf("cInput: %s",cInput);
    the printf shows me exactly what I want to see. Later, I use this to build a file path:
    Code:
      int k;
      char cFilename[17];
      char cPath[4] = "c:\\";
      char cAppend[5] = ".LOG";
      for(k=1;k<=31;k++)
      {
        if(k<10) snprintf(cFilename, sizeof(cFilename), "%s%s0%i%s",cPath,cInput,k,cAppend);
        else snprintf(cFilename, sizeof(cFilename), "%s%s%i%s",cPath,cInput,k,cAppend);
        printf("cInput: %s\n",cInput);
        printf("cFilename: %s\n",cFilename);
      }
    The two printf statements display exactly what I want to see:
    cInput: 201401
    cFilename: c:\20140101.LOG

    so, I open the file:

    Code:
        fp=fopen(cFilename, "r+"); //FILE *fp; was earlier defined outside of the loop
        if(fp == NULL) printf("Read File Error: %s", strerror(errno));
    another printf afterward to display works perfectly.

    I do a bunch of reads from the file, writing to some other files, never touching cInput or cFilename again. At the end of it, before looping again with my for loop, I print it out one last time:

    Code:
        printf("%s done\n\n\n",cFilename);
        printf("cInput: %s\n\n\n",cInput);
        printf("k: %i\n",k);
        printf("cAppend: %s\n",cAppend);
        system("PAUSE");
    Here's what I get:
    Code:
    C:\20140101.LOG done
    
    
    cInput: PE.ALT W33825 2014001 10 C6A ON PB04      C     @   8      E     @
        *** END OF DISPLAY ***+ @    23P043                     +
    
    
    k: 1
    cAppend: .LOG
    Press any key to continue . . .
    Any idea why that would be happening? Is there a better input (not scanf, obviously) that I could be using than the code I wrote for myself?
    Last edited by reillan; 01-30-2014 at 09:22 AM.

  2. #2
    Registered User
    Join Date
    Jan 2014
    Posts
    15
    Fun update:
    I tried copying the data out to another variable and copying it back in:
    Code:
    char cInputCopy[7];
    strcpy (cInputCopy,cInput);
    displaying both worked fine. Then at the end of the file I displayed both again and tried to copy back:
    Code:
        printf("cInput: %s\n\n\n",cInput);
        printf("cInputCopy: %s\n\n\n",cInputCopy);
        strcpy(cInput,cInputCopy);
    Now both end up corrupt:
    Code:
    cInput: 14001 10 C6A ON PB04      C     @   8      E     @          *** END OF D
    ISPLAY ***+ @    23P043                     + (
    
    
    cInputCopy: PE.ALT W33825 2014001 10 C6A ON PB04      C     @   8      E     @
            *** END OF DISPLAY ***+ @    23P043                     + (
    
    
    k: 1
    cAppend: .LOG
    and when I try to strcpy, the program crashes.

  3. #3
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    You talk about reading and writing to files as if that can't possibly affect any other variables. But if one of those reads overflows a buffer, that data can run into the other variables' memory.

    As for your input function:

    fflush(stdin) is non-standard, although if you're on a system that supports it (windows, at least with a windows compiler) it will not hurt anything. A more portable method is to read your first character with a scanf(" %c", &c) (note the space before the %c), which will skip leading whitespace if present, which is probably good enough.

    Why use atol to test if a character is a digit? Why not isdigit? And you're using it wrong anyway, since you're passing the address of the character as if that magically makes it a string. But it doesn't, since you are not guaranteed that there will just happen to be a zero byte after it on the stack.

    Using 0xd instead of '\n' is pointless and obfuscatory, as well as not fully portable.

    EDIT:
    And if the input you're reading is "20140101", then cInput is not large enough to hold it. In general, you are setting your character array sizes too tightly. No need to be so stingy. Give it some breathing room.

    Last edited by oogabooga; 01-30-2014 at 09:35 AM.
    The cost of software maintenance increases with the square of the programmer's creativity. - Robert D. Bliss

  4. #4
    Registered User
    Join Date
    Jan 2014
    Posts
    15
    I tried adding breathing room (a lot, actually) and it didn't help, sadly. I'm used to being stingy with them because when I was using scanf I'd get stack overflows literally nonstop... they take a toll on a man.

    I'm OK with windows being required - it's a windows-only environment.

    When I tried '\n' I got... funky results.

    But I did switch to !isdigit and it works just fine, so thank you for that.

    Here's the entire mess of code. There'll be a LOT wrong (as I've had a total of one month of training in C coming from a null state myself). I urge anyone looking to please try and focus on the string buffer overflow problem and not anything else

    I also have to redact anything that could reveal secure information, I'm afraid:
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    #include <errno.h>
    const char stats[40]="****************************************"; //redacted
    const char inits[40]="                                       ";
    
    void IterateDay(int date);
    int cInputString(int iMaxChar, char *cArr, int iValidate);
    
    int main(int argc, char *argv[])
    {
      int iDays = 0;
      char cInput[8];
      do
      {
        printf("Enter the Year and Month in YYYYMM format:\n");
        cInputString(6,cInput,2);
        if(cInput[4] == '0')
        {
          switch (cInput[5])
            {
              case '1':
              case '3':
              case '5':
              case '7':
              case '8': iDays = 31; break;
              case '4':
              case '6':
              case '9': iDays = 30; break;
              case '2':
                {
                  char cYear[4];
                  strncpy(cYear,cInput,4);
                  if(atoi(cYear)%4==0) iDays = 29;
                  else iDays = 28; 
                  break;
                }
            }
        }
        else if(cInput[4] == '1')
        {
          if(cInput[5] == '0' || cInput[5] == '2') iDays = 31;
          if(cInput[5] == '1') iDays = 30;
        }
      }
      while(iDays < 28);
      cInput[6] = '\0';
      printf("cInput: %s\n",cInput);
      char str[40]=" ";
      char spac[1]=" ";
      int ch;
      FILE *fp;
      FILE *finuse;
      FILE *ftpbs;
      finuse=fopen("c:\\finuse.txt", "w+");
      if(finuse == NULL) printf("Inuse Write File Error: %s", strerror(errno));
      fclose(finuse);
      ftpbs=fopen("c:\\ftpbs.txt", "w+");
      if(finuse == NULL) printf("TPBS Write File Error: %s", strerror(errno));
      fclose(ftpbs);
      char tim[9]="        "; //Initialize Time
      strcpy(str,inits);      //Initialize String
      char tp[3]="  ";        
      char bs[5]="    ";      
      char to[4]="   ";       
      char pc[3]="  ";        //Percent
      char inuse[7]="      "; //Total Resources Inuse
      int InStats=0;          //value to indicate position inside stats loop
      int i=0;                //Counter
      int k;
      char cFilename[80];
      char cPath[24] = "C:\\********************";  //redacted
      char cAppend[27] = "***********************.LOG"; //redacted
      for(k=1;k<=iDays;k++)
      {
        if(k<10)
        {                       
          snprintf(cFilename, sizeof(cFilename), "%s%s0%i %s",cPath,cInput,k,cAppend);
        }
        else snprintf(cFilename, sizeof(cFilename), "%s%s%i %s",cPath,cInput,k,cAppend);
        printf("cInput: %s\n",cInput);
        fp=fopen(cFilename, "r+");
        if(fp == NULL) printf("Read File Error: %s", strerror(errno));
        else
        {
          printf("Reading %s\n",cFilename);
          printf("cInput: %s\n",cInput);      
          system("PAUSE");
          while((ch=fgetc(fp)) != EOF)
          {
            if(i>11 && i<20) tim[i-12]=ch;
            if((ch=='\n' || ch=='\r') && !InStats) //if hitting \n while instats TRUE
                                                   //we need to store the stats first
              {
                if(strcmp(str,stats)==0) InStats=1;
                i=-1;
                strcpy(tim,"        ");
                strcpy(str,inits);
              }
            else if(i>40 && !InStats) str[i-41]=ch;
            else if(InStats)
              {
                if(ch=='\n' && InStats<6)
                  {
                    i=-1;
                    strcpy(tim,"        ");
                    InStats++;
                  }
                else if(ch!='\n' && InStats==6)
                  {
                    if(i>22 && i<25)
                      {
                        if(ch==32) InStats++;
                        else tp[i-23]=ch;
                      }
                    if(i>38 && i<43) bs[i-39]=ch;
                    if(i>53 && i<57) to[i-54]=ch;
                    if(i>64 && i<67) pc[i-64]=ch;
                  }
                else if((ch=='\n' || ch=='\r') && InStats==6)
                  {
                    ftpbs=fopen("c:\\ftpbs.txt", "a+");
                    fprintf(ftpbs,"%s, %s, %s, %s, %s\n",tim,tp,bs,to,pc);
                    fflush(ftpbs);
                    fclose(ftpbs);
                    i=-1;
                    strcpy(tim,"        ");
                    strcpy(tp,"  ");
                    strcpy(bs,"    ");
                    strcpy(to,"   ");
                    strcpy(pc,"  ");
                  }
                else if(ch=='\n' && InStats>6 && InStats < 10)
                  {
                    InStats++; 
                    i=-1;
                  }
                else if(ch!='\n' && InStats>9)
                  {
                    if(i>42 && i<49) inuse[i-43]=ch;
                  }
                else if(ch=='\n' && InStats>9)
                  {
                    finuse=fopen("c:\\finuse.txt", "a+");
                    fprintf(finuse,"%s, %s \n",tim,inuse);
                    fflush(finuse);
                    fclose(finuse);
                    InStats=0;
                    i=-1;
                  }
              }
            i++;
          }
      
        }
        fflush(stdin);
        fflush(fp);
        printf("%s done\n\n\n",cFilename);
        printf("cInput: %s\n\n\n",cInput);
    //    strcpy(cInput,cInputCopy);
        printf("k: %i\n",k);
        printf("cAppend: %s\n",cAppend);
        system("PAUSE");    
        fclose(fp); 
      }
      system("PAUSE");    
      return 0;
    }
    
    int cInputString(int iMaxChar, char *cArr, int iValidate)
    {
    /* This is a character input function to limit the length of characters
       inputted by the user.  This is necessary because without using such a 
       function, user could overrun the input stack and cause errors.  It'll 
       accept single-character inputs until it hits the number of characters 
       requested (iMaxChar) or until the user hits Enter. */
    /* iValidate determines whether to check for input.  Data is checked based on
       the following table:
       0: do not check data
       1: verify all data is alpha (lower and upper-case letters)
       2: verify all data is numeric */
    /* int return is the length of the characters inputted */
      fflush(stdin);
      char cC;
      int iCount = 0;
      do
        {
          cC = getch();
          printf ("%c",cC);
          switch (iValidate)
          {
            case 1:
            {
              if((isalpha(cC))||cC==' '||cC==0xd)
              {
                cArr[iCount] = cC;
                ++iCount;
                break;
              }
              else
              {
                printf("\nInvalid Entry, use only letters.  Please try again: ");
                break;
              }
            }
            case 2: 
              {   
              if (!isdigit(cC))
                {
                  printf("\nInvalid Entry, use only numbers.  Please try again: ");
                  break;
                }
              else if (cC == 0xd) break;
              else
                {
                  cArr[iCount] = cC;
                  ++iCount;
                  break;
                }
              }
            default:
              {                 
                cArr[iCount] = cC;
                ++iCount;
                break;
              }
          }
        }
      while (iCount < iMaxChar && cC != 0xd);
      printf ("\n");
      fflush(stdin);
      return iCount;
    }
    Last edited by reillan; 01-30-2014 at 10:20 AM.

  5. #5
    Registered User
    Join Date
    Jan 2014
    Posts
    15
    And I do want to say thank you much either way. I really appreciate any help I can get - it's so hard learning this language without any materials other than google

  6. #6
    Registered User
    Join Date
    Jan 2014
    Posts
    15
    I commented out a lot of the silly places where I strcpy spaces into a string, still works fine. Have to keep this one, I think:
    strcpy(str,inits);

    simply because if I didn't it'd write the first character and assume the rest were good with my following compare. I could probably write it a lot better, but... for now, that part works. That'll be version 0.2

    I increased all of the other strings:
    const char stats[45]
    const char inits[45]

    char tim[10]
    char tp[6]
    char bs[6]
    char to[6]
    char pc[6]

    //still works fine = still reads the first file, and still corrupts cInput afterward. heh.

  7. #7
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    You should include ctype.h (for isalpha and isdigit).

    You should use getchar (instead of the non-standard getch).

    spac is "unused" (and doesn't have enough space to hold a string with a single space, since that would require 2 characters, even with maximum stinginess ).

    If you want a character array to be the exact length necessary to hold a string constant used as an initializer, just leave the brackets blank.
    Code:
    const char stats[] = "****************************************";
    You don't need to flush a file just before you close it since fclose will flush it first. Actually, I don't see a reason for any of the flushes you're doing.

    Don't use + in the file opening modes unless you need it. It is used to both read and write from a file. If you're just doing one or the other, don't use +.

    When you check if a file has opened properly, you can't just print an error message and continue as if nothing happened. You have to do something about it. The easiest thing is to exit(EXIT_FAILURE).

    And try printing cFilename after the snprintf's. That might be interesting!
    The cost of software maintenance increases with the square of the programmer's creativity. - Robert D. Bliss

  8. #8
    Registered User
    Join Date
    Jan 2014
    Posts
    15
    Bwahahahhahaa

    Ok, so I finally thought "is that data coming from somewhere in my file?" and looked - there was a line much, much longer than expected, and it was overrunning my str[40] array. I increased it arbitrarily to 200 and the program works perfectly now.

    Thank you much!

  9. #9
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    To protect against such buffer overflows you can use fgets to read the file a line at a time. This function puts a limit on how much it reads, so it won't overflow the buffer. Since you have to pick a buffer size and can't be certain that a file will never have a line that's bigger, this is the best technique to protect against overflows. The only little snag is that fgets leaves the newline in the buffer (this is to indicate whether or not you were able to read a complete line). So you will often want to remove it:
    Code:
        char buf[BUFSIZ];
        FILE *fp = fopen("afile", "r");
    
        // read file line-by-line
        while (fgets(buf, sizeof buf, fp) != NULL) {
            // get rid of newline character
            char *p = strchr(buf, '\n');
            if (p) *p = '\0';
            // ...
        }
    You mentioned something about '\n' not working for you. Actually, I notice that you're using 0xd, which is not the same as '\n', which is (on most systems) 0xa. Anyway, you should try to use '\n'. Perhaps you accidentally used a forward slash instead of a backslash?
    The cost of software maintenance increases with the square of the programmer's creativity. - Robert D. Bliss

  10. #10
    Registered User hk_mp5kpdw's Avatar
    Join Date
    Jan 2002
    Location
    Northern Virginia/Washington DC Metropolitan Area
    Posts
    3,817
    Code:
    case '2':
    {
        char cYear[4];
        strncpy(cYear,cInput,4);
        if(atoi(cYear)%4==0) iDays = 29;
        else iDays = 28;
        break;
    }
    Your cYear array is not big enough to properly store/convert this data. atoi requires a null terminated character array so you'd need at least an array of size 5. strncpy also does not automatically append a null unless it reaches the end of the source string before 4 characters have been copied. In your case, if the source string is "201404" then calling strncpy(cYear,cInput,4) will still only assign the first 4 characters of cInput to cYear, the 5th character (index 4) will be whatever it was prior to the function call. This means you either have to null terminate it manually or initialize the array such that it already is filled with 0's.

    Your algorithm for determining if February is 29 or 28 days is also wrong. In this case, it is unlikely that a user is going to enter anything that would cause this problem to rear up but I'm just putting it out there. Simply validating that the year can be evenly divided by 4 is insufficient. Look here for some pseudocode. Your current code, for example, would count 2100 as a leap year when it is not.

    Your cInputString function does not null terminate the data it stores. Like what I said for cYear above, you either need to manually set this, or initialize cInput to all 0's. The function also returns a value but you don't seem to need or care about it, why have such a return value?

    I strongly recommend you avoid using fflush(stdin) in your code. It represents undefined behavior.
    "Owners of dogs will have noticed that, if you provide them with food and water and shelter and affection, they will think you are god. Whereas owners of cats are compelled to realize that, if you provide them with food and water and shelter and affection, they draw the conclusion that they are gods."
    -Christopher Hitchens

  11. #11
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    Quote Originally Posted by hk_mp5kpdw View Post
    I strongly recommend you avoid using fflush(stdin) in your code. It represents undefined behavior.
    "Undefined behavior" simply means that the standard does not define a behavior for it. That's all. Any particular implementation is free to define a behavior for it and still perfectly conform to the standard. Windows defines fflush(stdin) for the very useful task of emptying the input buffer. If the OP chooses to use that behavior, the only problem is that his code becomes non-portable. That is his choice.

    EDIT: I should add that "undefined behavior" also means that the implementation does not have to do anything predictable or even document what it does in that situation.
    Last edited by oogabooga; 01-30-2014 at 11:49 AM.
    The cost of software maintenance increases with the square of the programmer's creativity. - Robert D. Bliss

  12. #12
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    "Undefined behavior" simply means that the standard does not define a behavior for it.
    O_o

    Which is a good enough reason for anyone to warn the original poster off relying on undefined behavior.

    Seriously, I don't really get this post; you've warned people off undefined behavior yourself several times.

    Why defend `fflush(stdin)' with "The use should be his choice."?

    Using and teaching `fflush(stdin)' is garbage; it should be avoided.

    You even get it wrong; "Windows" does not define "fflush(stdin)" to do anything; the core "runtime" library shipped with Microsoft "Visual C++" libraries define the behavior, but other vendor implementations are not required to follow that behavior on "Windows".

    I'm not trying to be harsh, but warning people off undefined behavior is nearly the modus operandi for this forum.

    Soma
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

  13. #13
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    Which is a good enough reason for anyone to warn the original poster off relying on undefined behavior.
    I did "warn him off" by telling him it was non-portable.
    "Undefined behavior" is a portability issue. That was my point.
    I don't recall "teaching" (or using) fflush(stdin).


    You even get it wrong; "Windows" does not define "fflush(stdin)" to do anything; the core "runtime" library shipped with Microsoft "Visual C++" libraries define the behavior, but other vendor implementations are not required to follow that behavior on "Windows".
    I realize that and should have been more clear.
    The cost of software maintenance increases with the square of the programmer's creativity. - Robert D. Bliss

  14. #14
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    I don't recall "teaching" (or using) fflush(stdin).
    O_o

    No. I didn't say that; I was more referring to the fact that the original poster had to have picked it up from somewhere.

    More the point was "Squash it now and maybe it will not turn up later." which your post seemingly finds disagreeable.

    *shrug*

    Look, I wasn't trying to start anything; the post just seemed way out for you.

    Soma
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

  15. #15
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    I don't feel you're being harsh or trying to start anything. I'm interested in this discussion and I know that I have it at least partly wrong.

    I've been reading the standard lately and am trying to make sense of these kinds of details. I was surprised that it said the points where the standard leaves something undefined (or specifically states that it is undefined, which are supposed to be equivalent), that these points are places where an implementation can "extend" the standard in a portable way. I felt that fflush(stdin) was a reasonable extension in that spirit.

    To call fflush(stdin) "garbage" seems an emotional (anti-windows) reaction. I cannot think of a portable way to do what that function does. I gave a replacement that would skip whitespace, but I cannot think of a portable way to flush the input buffer that wouldn't hang if the buffer was empty. (But maybe I'm missing something.)

    EDIT:
    In my opinion, a sane gcc build for a windows environment should define fflush(stdin) to either do nothing or to do what the windows compiler runtime does.

    For example, on my machine with the gcc imp that I have, the following program shows that fflush(stdin) actually "flushes" the input buffer.
    Code:
    #include <stdio.h>
    
    int main() {
        int c;
    
        printf("Type a bunch of characters: ");
        c = getchar();
        (void)c; // "set but not used" suppression
    
        fflush(stdin);
    
        printf("Wait?\n");
        getchar();
    
        return 0;
    }
    Last edited by oogabooga; 01-30-2014 at 01:16 PM.
    The cost of software maintenance increases with the square of the programmer's creativity. - Robert D. Bliss

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 17
    Last Post: 01-29-2013, 07:58 PM
  2. Replies: 48
    Last Post: 02-12-2012, 02:33 PM
  3. Replies: 3
    Last Post: 10-21-2010, 12:39 PM
  4. Array fills with junk - help
    By Lucky Luke in forum C Programming
    Replies: 1
    Last Post: 12-13-2009, 05:13 PM
  5. Replies: 6
    Last Post: 06-30-2009, 09:37 AM