Re: [PATCH] Smackv10: Smack rules grammar + their stateful parser(2)

["Ahmed S. Darwish" <darwish.07@gmail.com>]

On Nov 10, 2007 7:05 PM, Jakob Oestergaard <jakob@unthought.net> wrote:
> ...
> > I've double-checked the code for any possible off-by-one/overflow
> > errors.
> ...
>
> Two things caught my eye.
>
> ...
> > +             case bol:
> > +             case subject:
> > +                     if (*label_len >= SMK_MAXLEN)
> > +                             goto out;
> > +                     subjectstr[(*label_len)++] = data[i];
>
> Why is the '>' necessary?  Could it happen that you had incremented past the
> point of equality?
>
> If that could not happen, then in my oppinion '>=' is very misleading when '=='
> is really what is needed.
>

Indeed, you're absolutely right.

> ...
> > +             case object:
> > +                     if (*prevstate == blank) {
> > +                             subjectstr[*label_len] = '\0';
> > +                             *label_len = 0;
> > +                     }
>
> I wonder why it is valid to uncritically use the already incremented label_len
> here, without checking its value (like is done above).
>
> It seems strangely asymmetrical. I'm not saying it's wrong, because there may
> be a subtle reason as to why it's not, but if that's the case then I think that
> subtle reason should be documented with a comment.
>

Maximum value label_len could reach is "SMK_MAXLEN". The subjectstr
and objectstr arrays are of "SMK_MAXLEN + 1" length. So I think no
danger is here.

Yes, this deserved a comment.

> ...
> > +             case access:
> > +                     if (*prevstate == blank) {
> > +                             objectstr[*label_len] = '\0';
> > +                             *label_len = 0;
> > +                     }
>
> Same applies here.
>
>  / jakob
>

Good spots, thanks a lot for the review.

Regards,

-- 
Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/