Undefined Behavior

[bbaale]

Why would this code cause undefined behavior?



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

#define TRUE 1
#define FALSE 0
#define MAX_PASSWORD 11
int loop;
unsigned char rx;


int _strlen(s)
unsigned char s[];
{
int i;

i=0;
while(s!='\0')
++i;
return i;
}

int encrypt (s, x, len)
unsigned char *s ;
unsigned char x;
int len;
{

int i,ok;
unsigned char seed_1,seed_2;

seed_1 = rx;
seed_2 = rx + 31;
ok = TRUE;
for(i = 0 ; i < len ; i++)
{
seed_1 -= i;
seed_2 += i;
if(i%2 != 0)
s -= seed_1;
else
s += seed_2;
if(s == '&' || s== '\0' || s == '\n')
ok = FALSE;
}
if (x == '&')
ok = FALSE;
return ok;
}



int decrypt (s, len)
unsigned char *s;
int len ;

{
unsigned char seed_1, seed_2;
int i;

seed_1 = rx;
seed_2 = rx+31;
for (i = 0; i < len ; i++)
{
seed_1 -= i;
seed_2 +=i;
if (i % 2 != 0)
s += seed_1;
else
s -= seed_2;
}
return TRUE;
}



int encrypt_string (s)
unsigned char *s ;
{

unsigned char x;
int c, len;

x=0;
c=FALSE;
len= _strlen(s);

while( c!= FALSE || loop>0)
{
x++;
c=encrypt(s, x, len);
loop--;
}
s[len++] = x;
s[len++] = rx;
s[len]= '\0';

return TRUE;
}


int decrypt_string(s)
unsigned char *s;
{

unsigned char x,y ;
int len;

len = _strlen(s)-1;

rx= s[len];
len--;
x=s[len];
for(y = 0; y < x ; y++)
decrypt(s, len);
s[len]= '\0';

return TRUE;
}


int main() {

unsigned char *s;
unsigned char endchar;
unsigned int y;
int len;
s=(unsigned char *) calloc( MAX_PASSWORD, sizeof(unsigned char) );
if(s==NULL){
printf("Could not allocate Memory");
return 0;
}

fgets(s,MAX_PASSWORD,stdin);
len=_strlen(s);

endchar =s[len-1];

if(endchar=='\r' || endchar=='\n')
s[len-1]='\0';

rx=strlen(s) ;
loop=1;

y=encrypt_string(s);

if(y==TRUE)
printf("\n\nEncrypted String :%s ######%d\n\n",s,_strlen(s));

y=decrypt_string(s);
if(y==TRUE)
printf("Decrypted String: %s %d\n",s,_strlen(s));

if(*s=!NULL){
free(s);
s=NULL;
}
else
printf("Could not deallocate memory\n");
return 0;

}

 

[Richard Heathfield]

(e-mail address removed) said:

Why would this code cause undefined behavior?

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

#define TRUE 1
#define FALSE 0
#define MAX_PASSWORD 11
int loop;
unsigned char rx;


int _strlen(s)

Invading implementation namespace.

 

[Richard Tobin]

#define MAX_PASSWORD 11

s=(unsigned char *) calloc( MAX_PASSWORD, sizeof(unsigned char) );
if(s==NULL){
printf("Could not allocate Memory");
return 0;
}

fgets(s,MAX_PASSWORD,stdin);
len=_strlen(s);

What's this _strlen()? Apart from anything else, that's a reserved
name.

endchar =s[len-1];

if(endchar=='\r' || endchar=='\n')
s[len-1]='\0';

So at this point s points to a string up to 11 bytes long (including
the nul at the end) in a buffer 11 bytes long.

y=encrypt_string(s);

int encrypt_string (s) ....
s[len++] = x;
s[len++] = rx;
s[len]= '\0';

Woops! You're adding things on to the end of s, and overflowing the
buffer.

You should be able to find mistakes like this yourself, using a
memory checking tool such as valgrind.

-- Richard

 

[CBFalconer]

Why would this code cause undefined behavior?

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

#define TRUE 1
#define FALSE 0
#define MAX_PASSWORD 11
int loop;
unsigned char rx;

int _strlen(s)
unsigned char s[];
{

Because right here you have used a label reserved for the
implementation. Don't do that.

 

[Martin Ambuhl]

Why would this code cause undefined behavior?

It was hard to resist rewriting this mess. Having resisted, I still
refuse to quote it all, except to note that

if(*s=!NULL){
free(s);
s=NULL;
}

probably is not what you meant.
if (*s = !NULL) ?* use some whitespace, for pete's sake */
does not mean the same thing as what you probably meant,
if (*s)

 

[Gregor H.]

if(*s=!NULL){
free(s);
s=NULL;
}
else
printf("Could not deallocate memory\n");

Should read


if (s != NULL)
{
free(s);
s = NULL;
}
else
printf("Could not deallocate memory\n");


I guess.


G.

 

[Christopher Benson-Manica]

(e-mail address removed) wrote:

you probably meant,
if (*s)

I wouldn't think so; I see no reason why a dynamically allocated
string shouldn't be freed simply because its length is 0.

 

[Martin Ambuhl]

I wouldn't think so; I see no reason why a dynamically allocated
string shouldn't be freed simply because its length is 0.

You're right. But guessing what the OP meant by his assignment
statement is up in the air. Any guess is likely to be wrong.

 

[Gregor H.]

You're right. But guessing what the OP meant by his assignment
statement is up in the air. Any guess is likely to be wrong.

Not so!

It should read


if (s != NULL)
{
free(s);
s = NULL;
}
else
printf("Could not deallocate memory\n");


I guess. (Such a construction -without the else-path- does make sense
_if_ it is possible that the allocated memory might have been freed
already at an earlier time. I once used such a construction, IIRC.)


G.

 

[Joachim Schmitz]

Not so!

It should read


if (s != NULL)
{
free(s);
s = NULL;
}
else
printf("Could not deallocate memory\n");


I guess. (Such a construction -without the else-path- does make sense
_if_ it is possible that the allocated memory might have been freed
already at an earlier time. I once used such a construction, IIRC.)

You can feed free() with anything that got returned by malloc(), calloc() or
realloc() which in turn can return NULL, so you can safely call free(NULL)
unconditionally and safe that exta code.

free(s),s = NULL;

The mesages "Could not deallocate memory" is a) not neccessary (as you
haven't got any memory to free in the first place) and b) if at all should
go to stderr, shouldn't it?

Bye, Jojo

 

[CBFalconer]

.... snip ...

It should read

if (s != NULL)
{
free(s);
s = NULL;
}
else
printf("Could not deallocate memory\n");

But, since free can operate on NULL, it can all be replaced by:

free(s);

 

[Gregor H.]

Not so!

It should read


if (s != NULL)
{
free(s);
s = NULL;
}
else
printf("Could not deallocate memory\n");


I guess. (Such a construction -without the else-path- does make sense
_if_ it is possible that the allocated memory might have been freed
already at an earlier time. I once used such a construction, IIRC.)

You can feed free() with anything that got returned by malloc(), calloc() or
realloc() which in turn can return NULL, so you can safely call free(NULL)
unconditionally and safe that exta code.

[This way we get:]

free(s)
s = NULL;

Ah, good to know!


G.

 

[Christopher Benson-Manica]

if (s != NULL)
{
free(s);
s = NULL;
}
else
printf("Could not deallocate memory\n");

I guess. (Such a construction -without the else-path- does make sense
_if_ it is possible that the allocated memory might have been freed
already at an earlier time. I once used such a construction, IIRC.)

I'm not sure what the benefit is. Aside from the fact that in OP's
code s had already been checked against NULL, if s is NULL there is no
memory to deallocate, making the error message rather strange. In
any case it seems to me that what is wanted is something like

void debug_free( void **ptr ) {
if( *ptr != NULL ) {
free( *ptr );
*ptr = NULL;
}
else {
fprintf( stderr, "Warning: ptr may have been free'd more than once");
}
}

Otherwise I see little point in not simply replacing the code with

free( s );
s = NULL;

 

[pete]

Not so!

It should read

if (s != NULL)
{
free(s);
s = NULL;
}
else
printf("Could not deallocate memory\n");

How is s going to equal NULL at that point in code,
when you've got This code first?:

s=(unsigned char *) calloc( MAX_PASSWORD, sizeof(unsigned char) );
if(s==NULL){
printf("Could not allocate Memory");
return 0;
}