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

[Jakob Oestergaard <jakob@unthought.net>]

...
> 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.

...
> +		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.

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

Same applies here.


-- 

 / jakob