[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1865922a0711101145g768fe96p74b400e31f4120a1@mail.gmail.com>
Date: Sat, 10 Nov 2007 21:45:06 +0200
From: "Ahmed S. Darwish" <darwish.07@...il.com>
To: "Jakob Oestergaard" <jakob@...hought.net>
Cc: "Casey Schaufler" <casey@...aufler-ca.com>, akpm@...l.org,
torvalds@...l.org, linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org, "Al Viro" <viro@....linux.org.uk>
Subject: Re: [PATCH] Smackv10: Smack rules grammar + their stateful parser(2)
On Nov 10, 2007 7:05 PM, Jakob Oestergaard <jakob@...hought.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists