CPqD / ofsoftswitch13

OpenFlow 1.3 switch.
http://cpqd.github.com/ofsoftswitch13
304 stars 192 forks source link

[bug report] dp_send_message double free cause core dump #278

Closed davidmpc closed 6 years ago

davidmpc commented 6 years ago

in the function dp_send_message, when send_openflow_buffer return error which is not 0, ofpbuf_delete delete the ofpbuf that is already delete in the rconn_send_with_limit function, this will cause double free and makes segment fault. So we should't delete the ofpbuf here.

ederlf commented 6 years ago

Please be a bit more specific about the problem.

In which case it happens? Which message returns error? Without details, it is even harder to reproduce an issue.

I believe a safe option to solve the issue is to reinit the buffer with a NULL base after deleting.

ofpbuf_init(b, 0);

should set the base and data fields of the ofpbuf struct to NULL.

davidmpc commented 6 years ago

Thanks for your reply. In my occasion, I got the error like this: Jul 30 14:26:23|17682|dp|WARN|send to passive failed: Resource temporarily unavailable Jul 30 14:26:23|17683|dp|WARN|There was an error sending the message! and I think it is because the function rconn_send_with_limit returns EAGAIN because "'n_queued' is already at least as large as 'queue_limit'". ofpbuf_init(b, 0); will set the base and data fields to NULL, but I have the doubt weather it works because b is still not NULL and ofpbuf_delete will free it again. Thanks.

ederlf commented 6 years ago

Thanks for the prompt reply! Ah, I see the case. Well, it might be worth setting b to NULL as well. It is better to make deletion safe, than adding additional checks (adding up complexity).

davidmpc commented 6 years ago

Yes it's ok to free NULL pointer but b as a pointer is not set to NULL in ofpbuf_init(b, 0) :)

davidmpc commented 6 years ago

Yes I agree with you and thank you very much! :+1: