[PATCH 00/12] p2p fixes

[Felipe Contreras]

Hi,

These patch series reorganizes the p2p to allow mainly two things:
1) properly check if directconn is enabled
2) free resources appropriately

Plus a few cleanups.

Felipe Contreras (12):
peer-msg: remove unused field
peer: remove internal msg
peer-msg: trivial reorg into got_transreq()
peer-msg: check if dc is enabled before starting
peer-msg: make pn_sip_recv() return bool
cvr: improve display of bogus messages
peer-link: clear messages after being parsed
peer-link: immediately unref msgs
peer-link: add warning on msg_ack()
peer-link: remove unused ref
peer-link: checks for lingering stuff
peer-link: improve handling of oversized msgs

cvr/pn_peer_link.c | 87 ++++++++++++++++++++--------
cvr/pn_peer_msg.c | 148 +++++++++++++++++++++++++++---------------------
cvr/pn_peer_msg.h | 6 +-
cvr/pn_peer_msg_priv.h | 4 -
4 files changed, 149 insertions(+), 96 deletions(-)

[Felipe Contreras]

Not used any more.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---
cvr/pn_peer_link.c | 3 ---
cvr/pn_peer_msg_priv.h | 2 --
2 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/cvr/pn_peer_link.c b/cvr/pn_peer_link.c
index e363ea9..a8f5afa 100644
--- a/cvr/pn_peer_link.c
+++ b/cvr/pn_peer_link.c
@@ -297,7 +297,6 @@ msg_ack(MsnMessage *msg,
}

leave:
- peer_msg->msgs = g_list_remove(peer_msg->msgs, msg);
pn_peer_msg_unref(peer_msg);
}

@@ -310,7 +309,6 @@ msg_nak(MsnMessage *msg,

peer_msg = data;

- peer_msg->msgs = g_list_remove(peer_msg->msgs, msg);
pn_peer_msg_unref(peer_msg);
}

@@ -352,7 +350,6 @@ send_msg_part(struct pn_peer_link *link,
#endif

pn_peer_msg_ref(peer_msg);
- peer_msg->msgs = g_list_append(peer_msg->msgs, msg);

#ifdef MSN_DIRECTCONN
/* The hand-shake message has 0x100 flags. */
diff --git a/cvr/pn_peer_msg_priv.h b/cvr/pn_peer_msg_priv.h
index 7bea326..38a90e1 100644
--- a/cvr/pn_peer_msg_priv.h
+++ b/cvr/pn_peer_msg_priv.h
@@ -54,8 +54,6 @@ struct pn_peer_msg {
guint64 offset;
guint64 size;

- GList *msgs; /**< The real messages. */
-
struct MsnMessage *msg; /**< The temporary real message that will be sent. */

#ifdef PECAN_DEBUG_SLP
--
1.6.6.1

[Felipe Contreras]

Not really used it seems.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

cvr/pn_peer_link.c | 29 ++++++++++++++---------------
cvr/pn_peer_msg_priv.h | 2 --
2 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/cvr/pn_peer_link.c b/cvr/pn_peer_link.c
index a8f5afa..da4123a 100644
--- a/cvr/pn_peer_link.c
+++ b/cvr/pn_peer_link.c
@@ -61,7 +61,10 @@ struct pn_peer_link {
unsigned int ref_count;
};

-static void send_msg_part(struct pn_peer_link *link, struct pn_peer_msg *peer_msg);
+static void
+send_msg_part(struct pn_peer_link *link,
+ struct pn_peer_msg *peer_msg,
+ MsnMessage *msg);

struct pn_peer_link *
pn_peer_link_new(MsnSession *session,
@@ -256,14 +259,15 @@ find_session_call(struct pn_peer_link *link,

static inline void
send_msg(struct pn_peer_link *link,
- struct pn_peer_msg *peer_msg)
+ struct pn_peer_msg *peer_msg,
+ MsnMessage *msg)
{
MsnSwitchBoard *swboard;
if (peer_msg->call)
swboard = peer_msg->call->swboard;
else
swboard = peer_msg->swboard;
- msn_switchboard_send_msg(swboard, peer_msg->msg, TRUE);
+ msn_switchboard_send_msg(swboard, msg, TRUE);
}

/* We have received the message ack */
@@ -284,7 +288,7 @@ msg_ack(MsnMessage *msg,
peer_msg->offset += msg->msnslp_header.length;

if (peer_msg->offset < real_size)
- send_msg_part(peer_msg->link, peer_msg);
+ send_msg_part(peer_msg->link, peer_msg, msg);
else {
/* The whole message has been sent */
if (peer_msg->flags == 0x20 ||
@@ -314,16 +318,12 @@ msg_nak(MsnMessage *msg,

static void
send_msg_part(struct pn_peer_link *link,
- struct pn_peer_msg *peer_msg)
+ struct pn_peer_msg *peer_msg,
+ MsnMessage *msg)
{
- MsnMessage *msg;
guint64 real_size;
size_t len = 0;

- /** @todo maybe we will want to create a new msg for this peer_msg instead of
- * reusing the same one all the time. */
- msg = peer_msg->msg;
-
real_size = (peer_msg->flags == 0x2) ? 0 : peer_msg->size;

if (peer_msg->offset < real_size) {
@@ -357,9 +357,9 @@ send_msg_part(struct pn_peer_link *link,
(peer_msg->flags == 0x100 || link->direct_conn->ack_recv))
pn_direct_conn_send_msg(link->direct_conn, msg);
else
- send_msg(link, peer_msg);
+ send_msg(link, peer_msg, msg);
#else
- send_msg(link, peer_msg);
+ send_msg(link, peer_msg, msg);
#endif /* MSN_DIRECTCONN */

if (peer_msg->call) {
@@ -385,7 +385,7 @@ release_peer_msg(struct pn_peer_link *link,
peer_msg->link = link;
link->slp_msgs = g_list_append(link->slp_msgs, peer_msg);

- peer_msg->msg = msg = msn_message_new_msnslp();
+ msg = msn_message_new_msnslp();

switch (peer_msg->flags) {
case 0x0:
@@ -431,7 +431,7 @@ release_peer_msg(struct pn_peer_link *link,
msg->nak_cb = msg_nak;
msg->ack_data = peer_msg;

- send_msg_part(link, peer_msg);
+ send_msg_part(link, peer_msg, msg);

msn_message_unref(msg);
}
@@ -630,7 +630,6 @@ pn_peer_link_process_msg(struct pn_peer_link *link,
peer_msg->ack_id = msg->msnslp_header.ack_id;
peer_msg->size = msg->msnslp_header.total_size;
peer_msg->flags = msg->msnslp_header.flags;
- peer_msg->msg = msg;

if (peer_msg->session_id) {
if (!peer_msg->call)
diff --git a/cvr/pn_peer_msg_priv.h b/cvr/pn_peer_msg_priv.h
index 38a90e1..0e6115d 100644

--- a/cvr/pn_peer_msg_priv.h
+++ b/cvr/pn_peer_msg_priv.h
@@ -54,8 +54,6 @@ struct pn_peer_msg {
guint64 offset;
guint64 size;

- struct MsnMessage *msg; /**< The temporary real message that will be sent. */
-
#ifdef PECAN_DEBUG_SLP
const gchar *info;
gboolean text_body;
--
1.6.6.1

[Felipe Contreras]

Makes it cleaner and easier to add code later. Besieds got_invite() is
too big already.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

cvr/pn_peer_msg.c | 117 +++++++++++++++++++++++++++-------------------------
1 files changed, 61 insertions(+), 56 deletions(-)

diff --git a/cvr/pn_peer_msg.c b/cvr/pn_peer_msg.c
index e55080d..29b123d 100644
--- a/cvr/pn_peer_msg.c
+++ b/cvr/pn_peer_msg.c
@@ -629,6 +629,65 @@ pn_sip_send_bye(struct pn_peer_call *call,
}

static void
+got_transreq(struct pn_peer_call *call,
+ const char *content,
+ const char *branch)
+{
+ const gchar *listening;
+ gchar *new_content, *nonce;
+
+ if (FALSE) {
+#if 0
+ struct pn_direct_conn *direct_conn;
+ /* const char *ip_addr; */
+ char *ip_port;
+ int port;
+
+ /* ip_addr = purple_prefs_get_string("/purple/ft/public_ip"); */
+ ip_port = "5190";
+ listening = "true";
+ nonce = pn_rand_guid();
+
+ direct_conn = pn_direct_conn_new(link);
+
+ direct_conn->nonce = g_strdup(nonce);
+
+ /* TODO: listen */
+
+ port = direct_conn->port;
+
+ new_content = g_strdup_printf("Bridge: TCPv1\r\n"
+ "Listening: %s\r\n"
+ "Nonce: {%s}\r\n"
+ "Ipv4Internal-Addrs: 192.168.0.82\r\n"
+ "Ipv4Internal-Port: %d\r\n"
+ "\r\n",
+ listening,
+ nonce,
+ port);
+#endif
+ }
+ else {
+ listening = "false";
+ nonce = g_strdup("00000000-0000-0000-0000-000000000000");
+
+ new_content = g_strdup_printf("Bridge: TCPv1\r\n"
+ "Listening: %s\r\n"
+ "Nonce: {%s}\r\n"
+ "\r\n",
+ listening,
+ nonce);
+ }
+
+ pn_sip_send_ok(call, branch,
+ "application/x-msnmsgr-transrespbody",
+ new_content);
+
+ g_free(new_content);
+ g_free(nonce);
+}
+
+static void
got_invite(struct pn_peer_call *call,
const char *branch,
const char *type,
@@ -664,62 +723,8 @@ got_invite(struct pn_peer_call *call,
g_free(context);
g_free(euf_guid);
}
- else if (strcmp(type, "application/x-msnmsgr-transreqbody") == 0) {
- /* A direct connection? */
-
- const gchar *listening;
- gchar *new_content, *nonce;
-
- if (FALSE) {
-#if 0
- struct pn_direct_conn *direct_conn;
- /* const char *ip_addr; */
- char *ip_port;
- int port;
-
- /* ip_addr = purple_prefs_get_string("/purple/ft/public_ip"); */
- ip_port = "5190";
- listening = "true";
- nonce = pn_rand_guid();
-
- direct_conn = pn_direct_conn_new(link);
-
- direct_conn->nonce = g_strdup(nonce);
-
- /* TODO: listen */
-
- port = direct_conn->port;
-
- new_content = g_strdup_printf("Bridge: TCPv1\r\n"
- "Listening: %s\r\n"
- "Nonce: {%s}\r\n"
- "Ipv4Internal-Addrs: 192.168.0.82\r\n"
- "Ipv4Internal-Port: %d\r\n"
- "\r\n",
- listening,
- nonce,
- port);
-#endif
- }
- else {
- listening = "false";
- nonce = g_strdup("00000000-0000-0000-0000-000000000000");
-
- new_content = g_strdup_printf("Bridge: TCPv1\r\n"
- "Listening: %s\r\n"
- "Nonce: {%s}\r\n"
- "\r\n",
- listening,
- nonce);
- }
-
- pn_sip_send_ok(call, branch,
- "application/x-msnmsgr-transrespbody",
- new_content);
-
- g_free(new_content);
- g_free(nonce);
- }
+ else if (strcmp(type, "application/x-msnmsgr-transreqbody") == 0)
+ got_transreq(call, content, branch);
#ifdef MSN_DIRECTCONN
else if (strcmp(type, "application/x-msnmsgr-transrespbody") == 0)
got_transresp(call, content);
--
1.6.6.1

[Felipe Contreras]

We were starting them blindly.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

cvr/pn_peer_msg.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/cvr/pn_peer_msg.c b/cvr/pn_peer_msg.c
index 29b123d..86a920f 100644
--- a/cvr/pn_peer_msg.c
+++ b/cvr/pn_peer_msg.c
@@ -291,6 +291,10 @@ got_transresp(struct pn_peer_call *call,
GList *list, *c;
GList *internal, *external;

+ if (!msn_session_get_bool(pn_peer_link_get_session(call->link),
+ "use_direct_conn"))
+ return;
+
listening = get_token(content, "Listening: ", "\r\n");
if (strcmp(listening, "true") != 0) {
/* nevermind, let's get it started */
@@ -636,6 +640,12 @@ got_transreq(struct pn_peer_call *call,
const gchar *listening;
gchar *new_content, *nonce;

+ /** @todo check actual type */
+
+ if (!msn_session_get_bool(pn_peer_link_get_session(call->link),
+ "use_direct_conn"))
+ return;
+
if (FALSE) {
#if 0
struct pn_direct_conn *direct_conn;
--
1.6.6.1

[Felipe Contreras]

Will be useful to report better warnings.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

cvr/pn_peer_msg.c | 17 ++++++++++++-----
cvr/pn_peer_msg.h | 6 ++++--
2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/cvr/pn_peer_msg.c b/cvr/pn_peer_msg.c
index 86a920f..33b4773 100644
--- a/cvr/pn_peer_msg.c
+++ b/cvr/pn_peer_msg.c
@@ -806,7 +806,7 @@ got_ok(struct pn_peer_call *call,
#endif /* MSN_DIRECTCONN */
}

-void
+gboolean
pn_sip_recv(struct pn_peer_link *link,
const char *body)
{
@@ -814,7 +814,7 @@ pn_sip_recv(struct pn_peer_link *link,

if (!body) {
pn_warning("received bogus message");
- return;
+ return FALSE;
}

/* show first line */
@@ -858,6 +858,8 @@ pn_sip_recv(struct pn_peer_link *link,
g_free(branch);
g_free(content_type);
g_free(content);
+
+ return TRUE;
}
else if (strncmp(body, "MSNSLP/1.0 ", strlen("MSNSLP/1.0 ")) == 0) {
char *content;
@@ -870,7 +872,7 @@ pn_sip_recv(struct pn_peer_link *link,
call = pn_peer_link_find_slp_call(link, call_id);
g_free(call_id);

- g_return_if_fail(call);
+ g_return_val_if_fail(call, FALSE);

if (strncmp(status, "200 OK", 6) != 0) {
/* It's not valid. Kill this off. */
@@ -893,7 +895,7 @@ pn_sip_recv(struct pn_peer_link *link,
pn_warning("received non-OK result: %s", temp);

pn_peer_call_unref(call);
- return;
+ return TRUE;
}

content_type = get_token(body, "Content-Type: ", "\r\n");
@@ -904,6 +906,8 @@ pn_sip_recv(struct pn_peer_link *link,

g_free(content_type);
g_free(content);
+
+ return TRUE;
}
else if (strncmp(body, "BYE", strlen("BYE")) == 0) {
char *call_id;
@@ -914,6 +918,9 @@ pn_sip_recv(struct pn_peer_link *link,

if (call)
pn_peer_call_unref(call);
- return;
+
+ return TRUE;
}
+
+ return FALSE;
}
diff --git a/cvr/pn_peer_msg.h b/cvr/pn_peer_msg.h
index d0c82a9..732afaf 100644
--- a/cvr/pn_peer_msg.h
+++ b/cvr/pn_peer_msg.h
@@ -20,6 +20,8 @@
#ifndef PN_PEER_MSG_H
#define PN_PEER_MSG_H

+#include <glib.h>
+
struct pn_peer_call;
struct pn_peer_link;

@@ -33,8 +35,8 @@ struct MsnMessage;
void pn_peer_msg_show(struct MsnMessage *msg);
#endif

-void pn_sip_recv(struct pn_peer_link *link,
- const char *body);
+gboolean pn_sip_recv(struct pn_peer_link *link,
+ const char *body);

void pn_sip_send_invite(struct pn_peer_call *call,
const char *euf_guid,
--
1.6.6.1

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

cvr/pn_peer_link.c | 5 ++++-
cvr/pn_peer_msg.c | 4 +---
2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/cvr/pn_peer_link.c b/cvr/pn_peer_link.c
index da4123a..264ab28 100644
--- a/cvr/pn_peer_link.c
+++ b/cvr/pn_peer_link.c
@@ -549,7 +549,10 @@ process_peer_msg(struct pn_peer_link *link,
else
{
body_str = g_strndup ((const char *) body, body_len);
- pn_sip_recv (link, body_str);
+ if (!pn_sip_recv (link, body_str)) {
+ pn_warning("'%s' sent a bogus message: [%s]:%li",
+ pn_peer_link_get_passport(link), body_str, peer_msg->flags);
+ }
}
g_free(body_str);
break;
diff --git a/cvr/pn_peer_msg.c b/cvr/pn_peer_msg.c
index 33b4773..b830049 100644
--- a/cvr/pn_peer_msg.c
+++ b/cvr/pn_peer_msg.c
@@ -812,10 +812,8 @@ pn_sip_recv(struct pn_peer_link *link,
{
struct pn_peer_call *call;

- if (!body) {
- pn_warning("received bogus message");
+ if (!body)
return FALSE;
- }

/* show first line */

{
--
1.6.6.1

[Felipe Contreras]

No need to keep the around; it's only a waste of resources.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

cvr/pn_peer_link.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/cvr/pn_peer_link.c b/cvr/pn_peer_link.c
index 264ab28..58ab6f8 100644
--- a/cvr/pn_peer_link.c
+++ b/cvr/pn_peer_link.c
@@ -736,11 +736,19 @@ pn_peer_link_process_msg(struct pn_peer_link *link,
break;
}

+ link->slp_msgs = g_list_remove(link->slp_msgs, peer_msg);
+ pn_peer_msg_unref(peer_msg);
+
if (call)
pn_peer_call_unref(call);

pn_peer_msg_unref(peer_msg);
}
+ else if (peer_msg->flags == 0x2) {
+ /* this is an ACK, lets just get rid of it */
+ link->slp_msgs = g_list_remove(link->slp_msgs, peer_msg);
+ pn_peer_msg_unref(peer_msg);
+ }
}

void
--
1.6.6.1

[Felipe Contreras]

Otherwise some of these will stay when the call is destroyed.

Also, there's no need to keep track of them; they'll be destroyed when
all the msg parts are fully sent or nak'ed.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

cvr/pn_peer_link.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cvr/pn_peer_link.c b/cvr/pn_peer_link.c
index 58ab6f8..8e7c675 100644
--- a/cvr/pn_peer_link.c
+++ b/cvr/pn_peer_link.c
@@ -383,7 +383,6 @@ release_peer_msg(struct pn_peer_link *link,
MsnMessage *msg;

peer_msg->link = link;
- link->slp_msgs = g_list_append(link->slp_msgs, peer_msg);

msg = msn_message_new_msnslp();

@@ -434,6 +433,7 @@ release_peer_msg(struct pn_peer_link *link,
send_msg_part(link, peer_msg, msg);

msn_message_unref(msg);
+ pn_peer_msg_unref(peer_msg);
}

void
--
1.6.6.1

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

cvr/pn_peer_link.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/cvr/pn_peer_link.c b/cvr/pn_peer_link.c
index 8e7c675..7a1d372 100644
--- a/cvr/pn_peer_link.c
+++ b/cvr/pn_peer_link.c
@@ -280,8 +280,10 @@ msg_ack(MsnMessage *msg,

peer_msg = data;

- if (!peer_msg->link)
+ if (!peer_msg->link) {
+ pn_warning("msg with no link?");
goto leave;
+ }

real_size = (peer_msg->flags == 0x2) ? 0 : peer_msg->size;

--
1.6.6.1

[Felipe Contreras]

No need for that since the same code created the message it holds a ref
to it.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

cvr/pn_peer_link.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/cvr/pn_peer_link.c b/cvr/pn_peer_link.c
index 7a1d372..6404b46 100644
--- a/cvr/pn_peer_link.c
+++ b/cvr/pn_peer_link.c
@@ -712,8 +712,6 @@ pn_peer_link_process_msg(struct pn_peer_link *link,
{
struct pn_peer_call *call = peer_msg->call;

- pn_peer_msg_ref(peer_msg);
-
if (!call)
peer_msg->swboard = user_data;

@@ -743,8 +741,6 @@ pn_peer_link_process_msg(struct pn_peer_link *link,

if (call)
pn_peer_call_unref(call);
-
- pn_peer_msg_unref(peer_msg);

}
else if (peer_msg->flags == 0x2) {

/* this is an ACK, lets just get rid of it */

--
1.6.6.1

[Felipe Contreras]

Otherwise they will leak.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

cvr/pn_peer_link.c | 28 ++++++++++++++++++++++++++++
1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/cvr/pn_peer_link.c b/cvr/pn_peer_link.c
index 6404b46..295cad8 100644
--- a/cvr/pn_peer_link.c
+++ b/cvr/pn_peer_link.c
@@ -92,6 +92,32 @@ pn_peer_link_new(MsnSession *session,
return link;
}

+static void
+remove_lingering(struct pn_peer_link *link)
+{
+ GList *l;
+
+ /* remove extra calls */
+ for (l = link->slp_calls; l; ) {
+ struct pn_peer_call *call = l->data;
+ l = l->next;
+
+ pn_info("remove lingering call: %p", call);
+ pn_peer_call_unref(call);
+ }
+ g_list_free(link->slp_calls);
+
+ /* remove extra slp_msgs */
+ for (l = link->slp_msgs; l; ) {
+ struct pn_peer_msg *peer_msg = l->data;
+ l = l->next;
+
+ pn_info("removing lingering slpmsg: %p", peer_msg);
+ pn_peer_msg_unref(peer_msg);
+ }
+ g_list_free(link->slp_msgs);
+}
+
void
pn_peer_link_free(struct pn_peer_link *link)
{
@@ -106,6 +132,8 @@ pn_peer_link_free(struct pn_peer_link *link)

session = link->session;

+ remove_lingering(link);
+
#ifdef MSN_DIRECTCONN
if (link->direct_conn)
pn_direct_conn_destroy(link->direct_conn);
--
1.6.6.1

[Felipe Contreras]

This does happen, let's not be fatalistic. Also, printing the size of
the msg would be useful for debugging. And finally, deal with it nicely
by clearing it since we are not going to handle it.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

cvr/pn_peer_link.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/cvr/pn_peer_link.c b/cvr/pn_peer_link.c
index 295cad8..1f8d2da 100644
--- a/cvr/pn_peer_link.c
+++ b/cvr/pn_peer_link.c
@@ -715,8 +715,10 @@ pn_peer_link_process_msg(struct pn_peer_link *link,
len = fwrite(data, 1, len, peer_msg->fp);
else if (peer_msg->size && peer_msg->buffer) {
if (len > peer_msg->size || offset > (peer_msg->size - len)) {
- pn_error("oversized peer_msg");
- g_return_if_reached();
+ pn_warning("oversized peer_msg: %zu", len);

+ link->slp_msgs = g_list_remove(link->slp_msgs, peer_msg);
+ pn_peer_msg_unref(peer_msg);

+ return;
}
else
memcpy(peer_msg->buffer + offset, data, len);
--
1.6.6.1