On Nov 10, 2007 7:05 PM, Jakob Oestergaard <[EMAIL PROTECTED]> 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-security-module" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
