Commit 01c613af authored by Nils Goroll's avatar Nils Goroll

micro-optimize backref when the reference is the subject tail

test is already included in c01.vtc
parent a289ca4f
......@@ -237,8 +237,11 @@ backref(VRT_CTX, VCL_INT refnum, VCL_STRING fallback, struct vmod_priv *task)
start = ov->subject + ov->ovector[refnum];
len = ov->ovector[refnum+1] - ov->ovector[refnum];
assert(len <= ov->ovector[1] - ov->ovector[0]);
WS_Assert_Allocated(ctx->ws, start, len);
substr = WS_Printf(ctx->ws, "%.*s", len, start);
WS_Assert_Allocated(ctx->ws, start, len + 1);
if (start[len] == '\0')
substr = start;
substr = WS_Printf(ctx->ws, "%.*s", len, start);
if (substr == NULL) {
VSLb(ctx->vsl, SLT_VCL_Error,
"vmod re: insufficient workspace");
  • This doesn't look right to me -- the static match function only copies strlen() of the subject string into workspace, if it has to copy, but not the terminating null:

    static VCL_BOOL
    match(VRT_CTX, vre_t *vre, VCL_STRING subject, struct vmod_priv *task,
    	const struct vre_limits *vre_limits)
    // ...
    	len = strlen(subject);
    // ...
    	if (WS_Inside(ctx->ws, subject, subject + len))
    		ov->subject = subject;
    	else if ((ov->subject = WS_Copy(ctx->ws, (const void *) subject, len))
    		 == NULL) {
    // ...

    If the WS_Inside() condition here is true, then maybe we have the terminating null, but maybe we don't -- we only check for the range subject to subject + len. It was intentional that backref() doesn't depend on having the terminating null (the ovector math in pcre doesn't require that).

    What part of c01.vtc did you think tests this? I suspect that either start[len] == '\0' in line 241 of the commit is never true, so only the else clause executes; or the condition is true "by accident", because that location of workspace might happen to have been null already. But not because it was intentionally null.

    We don't have any tests that use workspace right up to the limit, so the "out of workspace" conditions and tests for WS_Inside() and WS_Assert_Allocated() are never tested. If we did, then I suspect that the WS_Assert_Allocated() in line 240 would fail, because we only copied len bytes. not len + 1.

Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment