Lessons Learned && quiz (C/C++ programming)

[Maz]

Okay, I'll start writing up publishable mistakes I'll encounter during my career... Purpose is to

a) give you some puzzless:
and
b) give you some ideas of what kind of traps to avoid


Here's the first one.

Relevant part of the task:
I needed to replace some bytes from a number. During the runtime, I would get a number type of unsigned int. Now I do not know what the number would be like. I'll mark that number as 0xXXXXXXXX. Now, I should maintain all other bytes as they are, but change specified bytes to be something else. One example could be that I should change third and fourth byte to be AB. Now resulting number would be 0xXXABXXXX. Simple task I said...

I decided to do a function, which takes following parameters:
1. original number.
2. Bytes which should be changed
4. What to change the bytes.

Now in example abowe, I would give parameters
1. 0xXXXXXXXX
2. 0x00FF0000 //change bits forming 2nd and 3rd bytes
3. 0x00AB0000

Then I decided to use bitwise operations to form the correct result. I wrote following function:

Code: Select all

unsigned int changebytes(unsigned int const &real, unsigned int const &changedbits, unsigned int &changeto)
{
	unsigned int tmp,tmp2;
	tmp=(changedbits^(TAaSysComSicad)0x11111111); //get the bits which are not changed (bit's value will be inverted when XORed with bit 1)
	tmp2=(real & tmp); //null the bits which will be changed. (and will set result to zero, if one of the bits 'anded' is zero).
	return (tmp2 | changeto); //add changeto bits in the tmp (OR will 'sum' the bytes.
}

Now... What went wrong?

[Maz]

Here is the answer:

Code: Select all

/*.
.
.
.
.
.
.
.
.
Well, look at the line 
tmp=(changedbits^(TAaSysComSicad)0x11111111); //get the bits which are not changed (bit's value will be inverted when XORed with bit 1);

can you spot it now?
Ok. As you gathered it, answer is simple. 0x11111111 is not the number where all bits are set to 1. Therefor only part of the 'changedbits' is inverted. 0xFFFFFFFF would have been the correct number to use :) So correct function would have been
*/
unsigned int changebytes(unsigned int const &real, unsigned int const &changedbits, unsigned int &changeto)
{
   unsigned int tmp,tmp2;
   tmp=(changedbits^(TAaSysComSicad)0xFFFFFFFF); //get the bits which are not changed (bit's value will be inverted when XORed with bit 1)
   tmp2=(real & tmp); //null the bits which will be changed. (and will set result to zero, if one of the bits 'anded' is zero).
   return (tmp2 | changeto); //add changeto bits in the tmp (OR will 'sum' the bytes.
} 

Don't feel bad if you did not get it right away... I must admitt I was puzzled for a good deal of time. I even examined the assembly code with my debugger, and was almost accusing the processor for missbehaving

[Maz]

I almost made this mistake... I was starting to write this, when I suddenly noticed it wouldn't work...

Code: Select all

{
vector<object> objvector;

.
.
.
objvector.push_back(obj A);
.
.
objvector.push_back(obj B);
.
.
.
//Create thread with entrypoint void * newthreadfunc(obj *);
// give &objvector[N]
.
.
.
objvector.push_back(obj C)
.
.
.
}

void * newthreadfunc(obj *)
{
     //do things
     //do more things
     //Do something with objvector
     return something;
}

Obviously the purpose was to be able to use the same object in both threads. I would have done semaphores/mutexes and stuff, so that would not have been an issue... What would?

[Maz]

Answer is here

Answer lies within vector class' internal operation. When user uses push_back() to insert new items in vector, vector naturally allocates new space for item (if necessary), and stores the object in the vector.

Problem in this case is that vector guarantees that the space is continuous. Now, if there is no enough space for the item at the end of the vector's address space, vector will move all of it's contents into some other location on memory where continuous space is available. Naturally the raw pointer created and passed in new thread, will not be aware of the move, thus pointing at the old location which may be freed, or occupied by something else. Nasty bugs are to be expected, random crashes and generally some unexpected behaviour.

[Maz]

And then next one. At old days when things were simpler, I occasionally used to do pointer arithmetics by casting an address to integer, and after calculations back to a pointer. For example:

Code: Select all

void * get next_one(void **ptr, size_t sizeofobj)
{
      int address=(int)(*ptr); //store the address in integer
      address+=(int)sizeofobj;
      return (void *)address;
}

Who knows, why is this a bad idea nowadays? Again, answer will come at next week

[Maz]

Any guesses Pager?

[Pager]

I may be barking up the wrong tree, but is it because addresses can use a floating point number nowadays...so saving the address as an integer would round it up and result in the wrong address??

[Maz]

Almost correct Pager You were on right tracks!

Adresses cannot be floating points, but they may indeed be rounded. Or actually cutted. an integer is always 32 bits wide, for example (with hexa notation) it could be

0xB00BBABE

However, with modern 64 bit systems, addresses may be 64 bit wide. Thus casting a pointer to integer may cut the lower part of the address, and make it to point to invalid location. IE if I have address 0xABBAD0E5BEEFF00D, it would be truncated to
0xABBAD0E5. When this is casted back to pointer, it will be 0xABBAD0E500000000.
Naturally this leads the pointer to point in unknown location... And SFault knows what that causes

The way to go nowadays is to use long instead of int. Long is 32 bits wide on 32 bit systems, and 64 bits wide on 64 bit systems

[Pager]

SHAZAM! So close...

I always forget about the 64-bits...

[Pager]

what else you got Maz?

[Maz]

All right then. This one is just so easy basics of computer's operation, but so difficult to notice. Actually it is so difficult to notice, that I have done this mistake sozens of times, untill I recently heard about this obvious problem. And what's even worse, I am not the only one. I've heard that even some BSD kernel code has been patched because original version had a bug like this...

Anyways, question is, why this is a bad way to allocate memory for N structures:

In some header file we define a struct. For example

Code: Select all

typedef struct SStruct
{
    int integer;
    char character;
    char *char_array;
}SStruct;

And now the part where the bug lies:

Code: Select all

SStruct * allocate_sstruct_array(int amnt_of_structs)
{
       SStruct *the_array;
       the_array = malloc(amnt_of_structs*sizeof(SStruct));
       if(NULL==the_array)
       {
            fprintf(stderr,"Could not allocate the structs, out of mem?");
            return (SStruct *)NULL;
       }
       return the_array;
}

So, what simple thing is wrong with this?

[Maz]

Actually I assume we do not have any other C persons as active members, so after you have posted some answer, I'll reveal what I meant And Pager, if you have any similar things in mind, feel free to continue the list

[Maz]

Okay, I'll let the cat out of the bag.

amount_of_structs*sizeof(SStruct) may overflow if amount of structs is big enough. This is a nasty bug, because it results a number, smaller than originally intended. Thus malloc does reserve some space, but not enough.

Because malloc gets smaller number, and reserves the space, it thinks allocation was successfull and does not return NULL. Therefore the check for malloc failure at the next row does not result error.

There is no way detect that allocation failed, so nasty crashes and possibility for security issues does exist. Nasty Nasty overflowing

[Pager]

sorry Maz, I dropped the ball on that one...I went away to ponder it, and I think I got distracted by something shiny...

[Maz]

Okay, I'll give you another puzzle. This time it is significantly easier to spot, but still one of the 'easy to make' mistakes too. Here it goes:

Code: Select all

vector<int> vectorOfInts;
int i;
.
.
.
for(i=0;vectorOfInts[i]!=5&&(int)vectorOfInts.size()>i;i++)
{
       //do something
}

Here we do not know the size of the vector while entering in the for loop. What is the annoying bug in this?

[Maz]

And while I'm at it, I'll give you another one:

Code: Select all

int * returnPointerToInt(int x)
{
        int i;
        i=x*5;
        return &i;
}

This example (like the one abowe) look quite stupid. But the important thing in these is the principle which these examples introduce. There's some failure in the basic reasoning, and it can emerge in far more complicated, meaningfull examples too =)

[Pager]

For the first one, if you don't know the size, then the loop won't know when to stop, I'd imagine.

Let me think about the second one...I'm a little rusty on functionality of pointers...it usually happens when I haven't programmed for a while...

[Maz]

Mmm, I need to be a bit more accurate

I meant that we do not know how many items the vector will be holding when we enter at that loop. (at the time we write the program). Simple example would be that the vector is filled by numbers the user inputs, and that when user writes in - lets say - 72439, then the program would enter in that loop without pushing the 72439 in the vector.

But we have the vectorOfInts.size() call there. It returns the size of the vector (amount of items in it). So loop will terminate when all items have been checked. So what, what could possibly be wrong?

I can give you hints if you wish

[Maz]

Okay, I'll give you hints.

1. for the vector puzzle... Look at the order of operations in loop...

2. For the 'return pointer to a variable' puzzle, what is the scope of the? What happens when program leaves the scope of the i?

[Pager]

I think all this tech support stuff has finally eaten my brain.
Give me a couple of days...I gotta seriously brush up on my programming...