#1411 Disabling pipelining on multi handle with in-progress pipelined requests leads to heap corruption and crash

[Monty Brandenberg]

With 7.38.0 closing, I thought I'd get this report in now such as it is.

This is 7.37.0 on Win7/64-bit in a large application. A multi-handle is being used with a per-host connection limit of 8. Multi-handle may have up to 100 HTTP GET requests queued or in-progress. Pipelining is a dynamic setting in the application and can be toggled on and off at will. Dynamic change is implemented by communicating the change to the thread associated with the multi-handle which then enables or disables pipelining on the multi handle, no cross-thread issue should be involved.

Observed: with requests active on the multi handle, a transition from pipelining to non-pipelining is frequently followed by a sequence of errors. First will be an error from the library about missing server status (thought the error code was 9, not certain). That's followed shortly by a hard crash: typically calloc() throwing a runtime exception.

Suspected the library fails to honor in-progress/queued requests once pipelining is turned off so I did an application mod. When settings are changed, the multi handle is allowed to run until no requests are active then the multi options are set (enabling/disabling pipelining, etc.). This mod appears stable and reliable.

I imagine there's an easy fix which would simply be to continue processing the side request queues regardless of pipelining setting. Haven't checked that this is, in fact, the needed fix.

[Linus Nielsen]

I'm not sure what the preferred behaviour would be. Maybe it would help me with a further explanation of the use case. Why do you turn on and off pipelining dynamically?

[Monty Brandenberg]

1. Application is a long-running program whose users often need (or feel they need) to tweak networking behavior for their particular situation. They're often correct given the quality of ISPs around the globe and the latter's handling of HTTP, connection concurrency, etc.

2. libcurl itself is generally tolerant of dynamic changes in settings. They generally don't result in memory corruption and crashes. Variances from convention should be documented and, where they lead to invalid states, guarded against in code.

3. I think supporting this correctly will lead to less code, not more. Consistent handling of the connection send and recv queues when not empty and regardless of pipeline setting would be one result. Pipelining would only be considered when trying to find a connection for a request. That's my handwave, anyway.

[Linus Nielsen]

1) I see.

2) Of course libcurl should not crash or corrupt the heap. Nobody is suggesting that.

3) My question was how we should handle dynamic changes. We have two options, basically:

a) drop all outstanding requests. Might be a sane option if the user turned pipelining off because of a buggy server.

b) handle the outstanding requests gracefully and continue without pipelining any further requests.

I'll have a go at (b).

[Monty Brandenberg]

b) would be my preference. a) is harsh in any scenario, to do it without corrupting the protocol stream for requests sent but not received is doomed.

[Daniel Stenberg]

Confirmed, but now added to KNOWN_BUGS (as bug 89 in there) and will be closed as "later" here. We've not been able to deal with it until now and there's no work in sight. We of course appreciate anyone working on this and will re-open the minute someone wants to.