-
Notifications
You must be signed in to change notification settings - Fork 427
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
C2S crash fix #1597
C2S crash fix #1597
Conversation
The server crashes when variable IQ was not of iq record. Add check to prevent crash and return old state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a regression test for this.
src/ejabberd_c2s.erl
Outdated
end, | ||
Acc3 = ejabberd_router:route(To, From, Acc2, jlib:iq_to_xml(IQRes)), | ||
{Acc3, NewStateData}. | ||
case is_record(IQ, iq) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more common to write:
case IQ of
#iq{type = Type, sub_el = SubEl} = IQ ->
{Acc2...
What is more, this case
can even begin before line 2352.
Modify head of case construct in process_privacy_iq/3 Add regression tests for blocking and privacy namespaces
end, | ||
Acc3 = ejabberd_router:route(To, From, Acc2, jlib:iq_to_xml(IQRes)), | ||
{Acc3, NewStateData}. | ||
case mongoose_acc:get(iq_query_info, Acc) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use a separate function instead of case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, it's a tradeoff between large case and adding yet another function to ejabberd_c2s
(and coming up with a meaningful name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way works for me, the problem I have would be to come up with a meaningful function name.
@@ -68,7 +68,8 @@ effect_test_cases() -> | |||
messages_from_unblocked_user_arrive_again, | |||
messages_from_any_blocked_resource_dont_arrive, | |||
blocking_doesnt_interfere, | |||
blocking_propagates_to_resources | |||
blocking_propagates_to_resources, | |||
send_iq_with_blocking_ns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name doesn't tell precisely what the test does. Please consider iq_reply_doesnt_crash_user_process
or similar.
@@ -71,7 +72,8 @@ blocking_test_cases() -> | |||
block_jid_message_but_not_presence, | |||
newly_blocked_presense_jid_by_new_list, | |||
newly_blocked_presense_jid_by_list_change, | |||
newly_blocked_presence_not_notify_self | |||
newly_blocked_presence_not_notify_self, | |||
send_iq_with_privacy_ns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment in mod_blocking_SUITE
.
@@ -22,6 +22,7 @@ | |||
-include_lib("common_test/include/ct.hrl"). | |||
|
|||
-define(SLEEP_TIME, 50). | |||
-define(NS_PRIVACY, <<"jabber:iq:privacy">>). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defined already in escalus/include/escalus_xmlns.hrl
. You could actually include this header in mod_blocking_SUITE
and remove similar define
there.
escalus:story(Config, [{alice, 1}, {bob, 1}], fun(Alice, Bob) -> | ||
QueryWithPrivacyNS = escalus_stanza:query_el(?NS_BLOCKING, []), | ||
BobJid = escalus_utils:get_short_jid(Bob), | ||
IQError = escalus_stanza:iq(BobJid, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 295-306 may be extracted into parametrised helper in order to reduce code duplication. The same applies to privacy_SUITE
.
Remove previously defined definitions, include headerfiles Add generic helper function in privacy_helper module for new test cases
I am unsure about the placement of the new helper function in privacy_helper.erl |
Merging because only one test failed - resend on unacked, our known random. Thanks! |
This PR addresses crash of ejabberd_c2s process when receiving iq with blocking or privacy namespace
Proposed changes include:
Check if IQ is of record iq, if not return old state