Re: CC_send_query_append crash
От | Heikki Linnakangas |
---|---|
Тема | Re: CC_send_query_append crash |
Дата | |
Msg-id | 536746B6.50100@vmware.com обсуждение исходный текст |
Ответ на | Re: CC_send_query_append crash (Malcolm MacLeod <malcolm.macleod@tshwanedje.com>) |
Ответы |
Re: CC_send_query_append crash
|
Список | pgsql-odbc |
On 05/01/2014 02:47 PM, Malcolm MacLeod wrote: > >> <malcolm.macleod@tshwanedje.com> wrote: >>> The crash seems to occur because CC_send_query_append crash takes a >>> local copy of the pointer 'self->sock' at the top of the function, >>> 'self' is then passed around to various functions (some of which have >>> the side effect of setting self->sock to NULL (and deleting) if there is >>> a lost connection) and then the local copy of the pointer (which is now >>> dangling) is dereferenced lower down in the function. >>> Essentially if there is a disconnect while CC_send_query_append is >>> running there is a risk of crash. >> Looking at the code, I am seeing that the problem is related to >> CC_on_abort where conn->sock is set to NULL when the connection is >> considered as dead. And I am indeed seeing two code paths (when >> sending the 'C' message there is an ABORT check and in cleanup >> section) that could use this NULL socket afterwards. Your patch is >> perhaps a bit too much. So I am proposing the attached patch instead. >> Let me know if this fixes your issue as well. > > Thanks for the fast response! > Your proposed patch would also fix the issue, so I have no problem with > it being used instead. > > I guess from my side I just don't personally understand the point of > keeping the local pointer copy at all (it just seems like an invitation > for this sort of thing to occur) - so it made more sense to me to remove > it entirely to prevent future occurrences of similar issues - although I > suppose also the less code disturbed the better. I am not overly > familiar with the code so can't say what is best. I agree that keeping the local pointer copy is an accident waiting to happen. Michael's more surgical approach would make sense if we had to carefully track the places where self->sock might become NULL. But we don't, as all the SOCK_* macros and functions contain NULL checks. I think the extra null check that both patch versions added to the cleanup code path is also unnecessary and wrong. SOCK_get_errcode() returns SOCKET_CLOSED error if the argument is null, which will cause the connection to be marked as dead, which is the right thing to do. However, when I removed that check, the "diagnostics" regression test started to fail. What happens is that when the socket is closed while processing results in CC_send_query_append, the incomplete QResult object is discarded, even though it contained a useful error message. So we lose some information that way. But that in turn is pretty simple to fix: if we don't set ReadyToReturn in the cleanup routine, then the partial QResult object is returned again. Looking closer at the loop in CC_send_query_append(), I think it's basically wrong to set ReadyToReturn when an error occurs. ReadyToReturn indicates that the whole result set has been processed, and we're ready to send a new query. In many of the error conditions where we currently set ReadyToReturn, we're clearly not ready to process the next query. For example, in the 'T' case, if QR_Constructor() fails because of out-of-memory, we set ReadyToReturn=true and just bail out of the loop. That's wrong - we must process the rest of the result set before returning. Anyway, I committed your original patch with minor changes, which fixes your original problem (commit 22b151e). But the error handling in that function could use some more work... - Heikki
В списке pgsql-odbc по дате отправления: