Hi Joao,

On Thu, Aug 22, 2019 at 10:10:43PM -0300, Joao Morais wrote:
> 
> Hi list, I can reproduce a segmentation fault on HAProxy 1.8.21. No problem
> with 1.8.20, 1.9.10 or 2.0.5. Is there anything else I can provide or test on
> my environment?

Thank you for this report. The patch below looks highly suspicious to me :

  commit 72fdff1fdb5b82685dc3d2db23ece042195a0cbd
  Author: Christopher Faulet <cfau...@haproxy.com>
  Date:   Fri Apr 26 14:30:15 2019 +0200

    MINOR: spoe: Use the sample context to pass frag_ctx info during encoding
    
    This simplifies the API and hide the details in the sample. This way, only
    string and binary are aware of these info, because other types cannot be
    partially encoded.
    
    This patch may be backported to 1.9 and 1.8.
  (...)
@@ -2187,7 +2187,9 @@ spoe_encode_message(struct stream *s, struct spoe_context 
*ctx,
 
                /* Fetch the arguement value */
                smp = sample_process(s->be, s->sess, s, dir|SMP_OPT_FINAL, 
arg->expr, NULL);
-               ret = spoe_encode_data(&ctx->frag_ctx.curlen, smp, 
&ctx->frag_ctx.curoff, buf, end);
+               smp->ctx.a[0] = &ctx->frag_ctx.curlen;
+               smp->ctx.a[1] = &ctx->frag_ctx.curoff;
+               ret = spoe_encode_data(smp, buf, end);
                if (ret == -1 || ctx->frag_ctx.curoff)
                        goto too_big;
        }

As you can see, it doesn't test if smp is NULL when returning from
sample_process(), and surprisingly your trace shows exactly this
function and line.

Looking at it, 2.x don't have this issue. This was apparently fixed
by this commit :

  commit 3b1d004d410129efcf365643d2583dcd2cb6ed0f
  Author: Christopher Faulet <cfau...@haproxy.com>
  Date:   Mon May 6 09:53:10 2019 +0200

    BUG/MEDIUM: spoe: Be sure the sample is found before setting its context
    
    When a sample fetch is encoded, we use its context to set info about the
    fragmentation. But if the sample is not found, the function sample_process()
    returns NULL. So we me be sure the sample exists before setting its context.
    
    This patch must be backported to 1.9 and 1.8.

But despite being tagged for backport it was missed :-(

And that doesn't sound good, as apparently there was a long series of
patches merged in the same period and marked for backporting which were
forgotten, I'll have to have a look into this :-(

In the mean time you can apply the patch above. It will reject the
first hunk but the second one applies and will address the issue.

Thanks for reporting, and sorry for the messy backports, it looks like
1.8.22 will already have a bunch of patches in its queue...

Willy

Reply via email to