Messages in this thread | | | Date | Sat, 10 Nov 2007 21:45:06 +0200 | From | "Ahmed S. Darwish" <> | Subject | Re: [PATCH] Smackv10: Smack rules grammar + their stateful parser(2) |
| |
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/
| |