Skip to content
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.

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

Fix the invalid handling of message stanza in both tx and rx queue #171

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

root-hardenedvault
Copy link
Contributor

The xmpp packet (xmlnode ** msg_stanza_pp) should be handled very carefully,
and they apply different rules in sending and receiving queue.

In the sending queue of libpurple, because they are sent via
void jabber_send(JabberStream *js, xmlnode *packet) and released by the caller,
you could set (*msg_stanza_pp) to NULL, or modify (**msg_stanza_pp) in place,
but you should neither release (*msg_stanza_pp) (cause double free) nor point it
to another xmlnode. (there will be nowhere to release this xmlnode, since after
jabber_send() returns, the pointer value passed to its second argument packet
remain unchanged, no matter how packet is changed inside the context of
jabber_send(), because in C, arguments are passed to functions by value.)

In the receiving queue, on the other hand, they come from
void jabber_process_packet(JabberStream *js, xmlnode **packet), in this case,
you could modify (**msg_stanza_pp) in place, set (*msg_stanza_pp) to NULL, or
point it to another xmlnode (thus the new xmlnode will be released by the caller
of jabber_process_packet() ), but if you are going to point (*msg_stanza_pp) to
another place, you are responsible to release the original xmlnode in your
callback first, otherwise it will be leaked.

In order to modify the **msg_stanza_pp in place in the sending queue, a
dedicated function replace_msg_children() is added.

Signed-off-by: HardenedVault root@hardenedvault.net

The xmpp packet (xmlnode ** msg_stanza_pp) should be handled very carefully,
and they apply different rules in sending and receiving queue.

In the sending queue of libpurple, because they are sent via
void jabber_send(JabberStream *js, xmlnode *packet) and released by the caller,
you could set (*msg_stanza_pp) to NULL, or modify (**msg_stanza_pp) in place,
but you should neither release (*msg_stanza_pp) (cause double free) nor point it
to another xmlnode. (there will be nowhere to release this xmlnode, since after
jabber_send() returns, the pointer value passed to its second argument `packet`
remain unchanged, no matter how `packet` is changed inside the context of
jabber_send(), because in C, arguments are passed to functions by value.)

In the receiving queue, on the other hand, they come from
void jabber_process_packet(JabberStream *js, xmlnode **packet), in this case,
you could modify (**msg_stanza_pp) in place, set (*msg_stanza_pp) to NULL, or
point it to another xmlnode (thus the new xmlnode will be released by the caller
of jabber_process_packet() ), but if you are going to point (*msg_stanza_pp) to
another place, you are responsible to release the original xmlnode in your
callback first, otherwise it will be leaked.

In order to modify the **msg_stanza_pp in place in the sending queue, a
dedicated function replace_msg_children() is added.

Signed-off-by: HardenedVault <root@hardenedvault.net>
@codecov-commenter
Copy link

codecov-commenter commented May 18, 2021

Codecov Report

Merging #171 (f499541) into dev (38b0404) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff          @@
##             dev    #171   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          4       4           
  Lines       2111    2122   +11     
=====================================
- Misses      2111    2122   +11     
Impacted Files Coverage Δ
src/lurch.c 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38b0404...f499541. Read the comment docs.

@gkdr
Copy link
Owner

gkdr commented Jul 3, 2021

hi @root-hardenedvault, thanks again for the contribution.
thanks, good catch, in retrospect i don't know what i was thinking just replacing the value behind the pointer without freeing anything.
i don't quite understand why you can't just free the sent xmlnode like the received one, could you point out the libpurple code to me? i mean, it sounds like libpurple uses some weird way of keeping track of outgoing messages, leading to the double free, can you show me that? or is that easily reproducible by just doing that?

@root-hardenedvault
Copy link
Contributor Author

All outgoing xml packets is sent via jabber_send(), and by searching callings of jabber_send(), you can find they are all in form:

jabber_send(js, pkt);

xmlnode_free(pkt);

and jabber_send() is implemented in libpurple/protocols/jabber/jabber.c as such:

void jabber_send(JabberStream *js, xmlnode *packet)
{
	purple_signal_emit(purple_connection_get_prpl(js->gc),
	"jabber-sending-xmlnode",
	js->gc, &packet);
}

and &packet becomes your xmlnode ** msg_stanza_pp.

So, if xmlnode_free(*msg_stanza_pp) were performed, what will happen after jabber_send(js, pkt) returns, and xmlnode_free(pkt) is called?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants