Char array can't save chars as its members. Password system using ATmega8

Question

I was work with system, that read some symbols from the specific keyboard, pass it to the ATmega8, and then pass it to the display one-by-one(this step work correctly), but if I want to show all symbols array, I discovered that the dynamic array on first position save null, on the second save empty symbol, and the subsequent characters saved correctly. I don't see any mistakes in code, so I need a help.

This code function must return the 4-elements char array of symbols reading serial from the keyboard.

char* askPass(void){
     int i;
     char key;          
     #define PS 4
     char* pass = (char*)calloc(PS, sizeof(char));

     clear:
     lcd_clear();
     lcd_gotoxy(0, 0);
     lcd_puts("Enter the password:");  
     lcd_gotoxy(0, 1);               
     lcd_puts(">>");                          
     free(pass);
     pass = (char*)calloc(0, sizeof(char));
     for (i=0;i<PS;i++) pass[i] = ''; 

     for (i=0; i<PS; i++) {
         key = '-';
         key = readKey();
         lcd_gotoxy(3+i, 1);
         if (key == 'C') {
             goto clear;
         } else if (key == '-'){
             lcd_putchar('|');
             delay_ms(10);
             lcd_gotoxy(3+i, 1);
             lcd_putchar(' ');
             delay_ms(10);
             lcd_gotoxy(3+i, 1);
             i--;
         } else {
             pass = (char*)realloc(pass, i*sizeof(char));
             *(pass+i) = key;   
             lcd_putchar(*(pass+i));
             delay_ms(20);
         }            
     }             
     /// there is an error: 
      /// serial input: 1234
      /// lcd output: !* 34! 
      /// correct output: !1234!
     lcd_gotoxy(0,2);
     lcd_puts("!");       
     for (i=0; i<PS; i++) {
         if (!(*(pass+i))) lcd_putchar('*'); 
         else lcd_putchar(*(pass+i));
     }
     lcd_puts("!");                    
     // end error block 

     return pass;
 }  // can't return correct array value

/* All tests show this:

Serial input word: abcd
Output: !* cd!
Correct output: !abcd!

*/

Why do you use calloc if you have a fixed size array? Why do you realloc your array every time you add a character? Both is not needed. You should use a fixed size array either static or on the stack. — user6556709, Apr 28 '19 at 19:52
Which language, C **or** C++? They are distinct languages. For example, C++ has `std::string` for text and the `std::stream` class, and you can overload functions. The C language only has arrays of char, which can overflow and needs a terminating NUL character. — Thomas Matthews, Apr 28 '19 at 20:09
Prefer to use static or auto declared arrays in embedded systems. Using dynamic memory may lead to fragmentation. — Thomas Matthews, Apr 28 '19 at 20:11
I recommend passing a pointer to a character array (and the capacity of the array) to the function. Let the caller manage the memory. Since you tagged C++, you should be using `std::string` for the text (pass by reference so you can modify the caller's string). — Thomas Matthews, Apr 28 '19 at 20:13
You have a memory leak. Before the label `clear`, you allocate memory for `pass`. After the label, you allocate memory for `pass` again. The memory from the first allocation is now lost (a.k.a. memory leak). Another memory leak occurs from the `goto clear` statement (the previous allocation is lost with the next allocation of `pass`). — Thomas Matthews, Apr 28 '19 at 20:17
You should consider your attempt a failure if you thing a `goto` is a good idea in this solution. — Clifford, Apr 28 '19 at 22:31

Clifford · Answer 1 · 2019-04-28T22:33:15.740

You start my allocating 4 bytes with:

char* pass = (char*)calloc(PS, sizeof(char));

Then you call free(pass) before you even use the allocated data, only to then allocates zero bytes with:

pass = (char*)calloc(0, sizeof(char));

If makes no sense to allocate only to free without using the allocation, or to allocate zero bytes. The act of allocating zero bytes is undefined, but nothing good or useful will happen by attempting to write to such an allocation as you do.

Continuously reallocation to add one byte at a time is ill-advised also. It would be better to allocate a buffer of a reasonable length to start with and if you need to extend, extend by a number of characters in on chunk.

It is unclear in any case why you are dynamically allocating and reallocating to allow input of an arbitrary number of characters that you are not using. It would be simpler and more deterministic, to simply accept the four characters in a fixed length array and discard any extraneous input.

There is no such thing as an "empty character", calloc already initialised the allocation to zero (a nul character).

I am pretty certain this code has many other issues, but it is had to determine what you are trying to do in order to advise; you would well to use a debugger.

Char array can't save chars as its members. Password system using ATmega8

1 Answers1