Skip to content

Fix for AsyncUDP's packet handling callback #3290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

Fix for AsyncUDP's packet handling callback #3290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7cd492f
A first go at implementing a fix for AsyncUDP's packet handling callb…
ssilverman Sep 27, 2019
46010ed
Fixing the "_remotePort" missing '_'.
ssilverman Sep 27, 2019
707cd90
Merge remote-tracking branch 'upstream/master' into fix-asyncudp-pack…
ssilverman Oct 5, 2019
221c38e
Merge branch 'master' into fix-asyncudp-packet-handler
ssilverman Oct 29, 2019
80496e6
Merge branch 'master' into fix-asyncudp-packet-handler
ssilverman Nov 16, 2019
2673c4f
Merge branch 'master' into fix-asyncudp-packet-handler
ssilverman Mar 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions libraries/AsyncUDP/src/AsyncUDP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,22 @@ AsyncUDPPacket::AsyncUDPPacket(AsyncUDP *udp, pbuf *pb, const ip_addr_t *raddr,
}
}

AsyncUDPPacket::AsyncUDPPacket(const AsyncUDPPacket &packet)
: _udp(packet._udp),
_pb(packet._pb),
_if(packet._if),
_localIp(packet._localIp),
_localPort(packet._localPort),
_remoteIp(packet._remoteIp),
_remotePort(packet._remotePort),
_data(packet._data),
_len(packet._len),
_index(packet._index)
{
memcpy(_remoteMac, packet._remoteMac, sizeof(_remoteMac));
pbuf_ref(_pb);
}

AsyncUDPPacket::~AsyncUDPPacket()
{
pbuf_free(_pb);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here the pbuf should be freed. should take care of pbuf_ref as on that depends if pbuf_free will work

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is freed, as you point out, in the destructor. This balances the call to pbuf_ref in the constructor. That's why my change also adds a call to pbuf_ref in the copy constructor. This solves the imbalance.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written, the code won't successfully work for both pass-as-reference and pass-as-copy forms, no matter where the pbuf_free call is used in AsyncUDP::_recv, either outside or inside an else. That's why adding to the copy constructor solves the problem for both cases, if the pbuf_free is moved outside the if/else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried your fix before and it did not work in many situations. so I reverted it back. you could look in the history... should be there :) need to figure out better that pbuf ref/unref count and not leak or double free

Copy link
Author

@ssilverman ssilverman Sep 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Could you show me some situations in which my fix fails? I’d like to see if I can resolve them. All I know is that my fix doesn’t fail in any of my tests.
Unless... what are you saying about my fix doesn’t work? From browsing your other code, it looks like you just moved around the pbuf_free In AsybcUDPPacket::_recv without adding code to the copy constructor that calls pbuf_ref. My fix consists of two parts: moving around that pbuf_free call outside the else and modifying the copy constructor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately I can not recall...

Copy link
Author

@ssilverman ssilverman Oct 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you’re saying that in the past you’ve already tried adding this fix with changing the copy constructor and it didn’t work? What do you think of the logic in #3288? I don’t see what’s wrong with it.

Could you at least show which commits you undid where the fix didn’t work?

[I’m not saying my solution is not wrong or incorrect, I just don’t see its error. I’d love to help fix this.]

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@me-no-dev ?

Expand Down Expand Up @@ -682,9 +698,8 @@ void AsyncUDP::_recv(udp_pcb *upcb, pbuf *pb, const ip_addr_t *addr, uint16_t po
if(_handler) {
AsyncUDPPacket packet(this, this_pb, addr, port, netif);
_handler(packet);
} else {
pbuf_free(this_pb);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on destruct the packet will free the pbuf. if it does not, that should be fixed there, or where the packet is copied

Copy link
Author

@ssilverman ssilverman Sep 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite following what you're saying. pbuf_free, according to the code, needs to be called at least once, even if the handler isn't used. As soon as the handler is used with the new AsyncUDPPacket object, its constructor and destructor will pbuf_ref and pbuf_free, respectively. That's why the copy constructor needs to add a pbuf_ref, to balance things out if the pass-as-copy form of the callback is used.

What are your thoughts on the logic in #3288? I tested the code with my example from #3287 using both pass-as-reference and pass-as-copy forms.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possibility for a fix: Don't call pbuf_ref in the constructor or copy constructor, and don't call pbuf_freein the destructor. Have all management outside the AsyncUDPPacket object. However, code that currently exists and assumes the class will manage this will break.

}
pbuf_free(this_pb);
}
}

Expand Down
1 change: 1 addition & 0 deletions libraries/AsyncUDP/src/AsyncUDP.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ class AsyncUDPPacket : public Stream
size_t _index;
public:
AsyncUDPPacket(AsyncUDP *udp, pbuf *pb, const ip_addr_t *addr, uint16_t port, struct netif * netif);
AsyncUDPPacket(const AsyncUDPPacket &packet);
virtual ~AsyncUDPPacket();

uint8_t * data();
Expand Down