[Freeipa-devel] Re: [PATCH] COLLECTION patches resending

[Dmitri Pal]

Stephen Gallagher wrote:
> On 07/10/2009 05:29 PM, Dmitri Pal wrote:
> >>>> These unsigned values are the size of the internal stack or depth
> level.
> >>>> They need to  always be positive this is why they are unsigned.
> >>>> I do not want obfuscate the error by preventing them to go below 0.
> >>>> If it goes the program will crash and we will see the problem.
> >>>>       
> >> Are you sure that the program will crash or maybe just override some
> >> random memory? If you use a macro you can call abort() if the value is
> >> 0 and the program will definitely exit.
> >>
> >>   
> > It will become MAX unsigned. Since the variable is used as an index to
> > the array it will point to some unrelated memory and program will get
> > memory violation.
>
> You cannot and MUST NOT rely on a "memory violation", since it's
> possible for you to be working on a machine with copious amounts of
> memory, where max unsigned might still be addressable (see PAE kernels
> for examples of this). Sumit is correct, you need to do this with a
> macro that calls abort() if a program attempts to decrement the stack
> counter when it's already zero.
>
I do not agree with your argument and I do not like the abort().
IMO it is a theoretical discussion anyways. We are talking about  bad
things happening if there is a bug.
The whole point is not to hide it if it is there. But the bigger point
is not to have a bug.
The latest patch addresses all the issues with "--".

If you look at the code closely you will see that:
1) In walk_items the depth is incremented at the entry to the function
and decremented at exit - no check is needed there. 
2) In the iterator the check happens at the top of the loop and at the
beginning the stack_size is set to 1.
3) The only problem was in the iterate_up function that is now reworked
with the second patch.

Please review and ack/nack.

Thanks
Dmitri