Hi,
On 21.02.2013 15:33, Jan Kiszka wrote:
> On 2013-02-15 12:00, Vitaly Chipounov wrote:
>> A socket may still have references to it in various queues
>> at the time it is freed, causing memory corruptions.
> Did you see it in practice? Or is this patch based on code review? What
> will happen if those queued mbufs find their ifq_so NULL?
I have a packet trace that triggers this problem when it is injected
into the guest's NIC.
I am still not quite sure why this happens, but suspect that it could be
caused by malformed/partial TCP/IP streams.
Setting ifq_so to NULL shouldn't be an issue because there seems to be
checks for that (e.g., in if.c).
It doesn't seem to affect normal web browsing/downloading in the guest.
Vitaly
>
>> Signed-off-by: Vitaly Chipounov <vitaly.chipou...@epfl.ch>
>> ---
>> slirp/socket.c | 29 +++++++++++++++++++++++++++++
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/slirp/socket.c b/slirp/socket.c
>> index 77b0c98..8a7adc8 100644
>> --- a/slirp/socket.c
>> +++ b/slirp/socket.c
>> @@ -55,6 +55,33 @@ socreate(Slirp *slirp)
>> return(so);
>> }
>>
>> +/**
>> + * It may happen that a socket is still referenced in various
>> + * mbufs at the time it is freed. Clear all references to the
>> + * socket here.
>> + */
>> +static void soremovefromqueues(struct socket *so)
>> +{
>> + if (!so->so_queued) {
>> + return;
>> + }
>> +
>> + Slirp *slirp = so->slirp;
>> + struct mbuf *ifm;
>> +
>> + for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm =
>> ifm->ifq_next) {
> This line and the one below smells like checkpatch.pl wasn't run against
> it. Granted, slirp patches easy make checkpatch upset as the input is
> not properly formatted, but please don't add more violations.
>
>> + if (ifm->ifq_so == so) {
>> + ifm->ifq_so = NULL;
>> + }
>> + }
>> +
>> + for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm =
>> ifm->ifq_next) {
>> + if (ifm->ifq_so == so) {
>> + ifm->ifq_so = NULL;
>> + }
>> + }
>> +}
>> +
>> /*
>> * remque and free a socket, clobber cache
>> */
>> @@ -79,6 +106,8 @@ sofree(struct socket *so)
>> if(so->so_next && so->so_prev)
>> remque(so); /* crashes if so is not in a queue */
>>
>> + soremovefromqueues(so);
>> +
>> free(so);
>> }
>>
>>
> Sorry for the late feedback,
> Jan
>
