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 issue with max_stanza_size #4197

Merged
merged 5 commits into from
Dec 22, 2023
Merged

Fix the issue with max_stanza_size #4197

merged 5 commits into from
Dec 22, 2023

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Dec 21, 2023

The limit worked partially - only if the input was chunked, and not working properly for small limits. For websockets it wasn't working at all as reported in #4193.

The main fix is in the C++ code in exml: esl/exml#68
This PR also uses a fix for escalus - I discovered an issue when adding a new test: esl/escalus#261

These two PR's should be merged first. Then we could release them in hex, and update this PR.

Note: Latest escalus does not work with big tests anymore after merging esl/escalus#259, and this needs a fix before releasing the hex package.


Recommended to do in the future:

  • Add tests for the opening stream tag name verification in both regular C2S and websockets.

@chrzaszcz chrzaszcz force-pushed the fix-max-stanza-size branch 5 times, most recently from 265937e to ba8a7a3 Compare December 21, 2023 20:37
@mongoose-im

This comment was marked as outdated.

Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (dc7cef6) 84.29% compared to head (309fb47) 84.33%.

Files Patch % Lines
src/mod_websockets.erl 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4197      +/-   ##
==========================================
+ Coverage   84.29%   84.33%   +0.03%     
==========================================
  Files         549      549              
  Lines       33419    33419              
==========================================
+ Hits        28171    28183      +12     
+ Misses       5248     5236      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- Replace max_child_size with max_element_size
- Replace 'child element too big' with 'element too big'
- Remove unsupported and obsolete exml options: autoreset, start_tag
- Test with websockets, because they don't get chunked input and were
  failing before fixing exml.
- Test the limit for stream opening tag as well.
- exml has the fix for max_element_size
- escalus has the fix for handling the 'close' element in websockets
@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz marked this pull request as ready for review December 22, 2023 07:43
@chrzaszcz chrzaszcz marked this pull request as draft December 22, 2023 07:46
Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

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

Pushed the latest escalus and exml packages together with fixes to the jid record problem. Now it looks perfect to me 👌🏽

@mongoose-im
Copy link
Collaborator

mongoose-im commented Dec 22, 2023

elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / 309fb47
Reports root/ big
OK: 370 / Failed: 0 / User-skipped: 40 / Auto-skipped: 0


small_tests_25 / small_tests / 309fb47
Reports root / small


small_tests_26 / small_tests / 309fb47
Reports root / small


small_tests_26_arm64 / small_tests / 309fb47
Reports root / small


ldap_mnesia_25 / ldap_mnesia / 309fb47
Reports root/ big
OK: 2271 / Failed: 0 / User-skipped: 847 / Auto-skipped: 0


dynamic_domains_mysql_redis_26 / mysql_redis / 309fb47
Reports root/ big
OK: 4209 / Failed: 0 / User-skipped: 135 / Auto-skipped: 0


ldap_mnesia_26 / ldap_mnesia / 309fb47
Reports root/ big
OK: 2271 / Failed: 0 / User-skipped: 847 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 309fb47
Reports root/ big
OK: 4241 / Failed: 1 / User-skipped: 102 / Auto-skipped: 0

sm_SUITE:parallel_manual_ack_freq_1:resume_session_state_send_message_without_ack
{error,
  {{assertion_failed,assert,is_presence,
     {xmlel,<<"message">>,
       [{<<"from">>,
         <<"bob_resume_session_state_send_message_without_ack_2972@domain.example.com/escalus-default-resource">>},
        {<<"to">>,
         <<"alice_resume_session_state_send_message_without_ack_2979@domain.example.com">>},
        {<<"type">>,<<"chat">>}],
       [{xmlel,<<"body">>,[],[{xmlcdata,<<"msg-3">>}]}]},
     "<message from='bob_resume_session_state_send_message_without_ack_2972@domain.example.com/escalus-default-resource' to='alice_resume_session_state_send_message_without_ack_2979@domain.example.com' type='chat'><body>msg-3</body></message>"},
   [{escalus_new_assert,assert_true,2,
      [{file,
         "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
       {line,84}]},
    {sm_helper,initial_presence_step,2,
      [{file,"/home/circleci/project/big_tests/tests/sm_helper.erl"},
       {line,135}]},
    {escalus_connection,connection_step,2,
      [{file,
         "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_connection.erl"},
       {line,163}]},
    {lists,foldl_1,3,[{file,"lists.erl"},{line,1355}]},
    {escalus_connection,start,2,
      [{file,
         "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_connection.erl"},
       {line,145}]},
    {sm_helper,connect_spec,3,
      [{file,"/home/circleci/project/big_tests/tests/sm_helper.erl"},
       {line,153}]},
    {sm_S...

Report log


internal_mnesia_26 / internal_mnesia / 309fb47
Reports root/ big
OK: 2411 / Failed: 0 / User-skipped: 707 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / 309fb47
Reports root/ big
OK: 4242 / Failed: 0 / User-skipped: 102 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / 309fb47
Reports root/ big
OK: 4239 / Failed: 0 / User-skipped: 105 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 309fb47
Reports root/ big
OK: 4631 / Failed: 0 / User-skipped: 109 / Auto-skipped: 0


pgsql_cets_26 / pgsql_cets / 309fb47
Reports root/ big
OK: 4286 / Failed: 0 / User-skipped: 110 / Auto-skipped: 0


pgsql_mnesia_26 / pgsql_mnesia / 309fb47
Reports root/ big
OK: 4631 / Failed: 0 / User-skipped: 109 / Auto-skipped: 0


mysql_redis_26 / mysql_redis / 309fb47
Reports root/ big
OK: 4610 / Failed: 0 / User-skipped: 130 / Auto-skipped: 0


mssql_mnesia_26 / odbc_mssql_mnesia / 309fb47
Reports root/ big
OK: 4628 / Failed: 0 / User-skipped: 112 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 309fb47
Reports root/ big
OK: 4242 / Failed: 0 / User-skipped: 102 / Auto-skipped: 0

@NelsonVides NelsonVides marked this pull request as ready for review December 22, 2023 17:35
Copy link
Member Author

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

Looks all good to me, nice to see the includes fixed, it was easier than I thought. I saw two test suites, but it turned out these were the only two 😉
From my side it can be merged 👍

@NelsonVides NelsonVides merged commit 8775cb9 into master Dec 22, 2023
4 checks passed
@NelsonVides NelsonVides deleted the fix-max-stanza-size branch December 22, 2023 18:08
@jacekwegr jacekwegr added this to the 6.2.1 milestone Apr 3, 2024
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.

4 participants