Skip to content
This repository has been archived by the owner on Nov 17, 2020. It is now read-only.

Make pmon:demonitor/2 respect its contract #18

Merged

Conversation

binarin
Copy link
Contributor

@binarin binarin commented Nov 19, 2015

Spec states that demonitor/2 should always return #state{}, but it
wasn't the case when pid wasn't found in #state.dict. This made API
unsafe to use, as it could lead to some other process storing incorrect
data as a pmon state.

This patch will prevent exceptions with stacktrace provided at the end of this message, where lists:foldl/3 was used to accumulate result of sequence of pmon:demonitor/2 applications.

Probably attempt to demonitor something that was not monitored is manifestation of another bug, but I wasn't able to pinpoint it.

** {function_clause,
       [{pmon,demonitor,
            [<23061.2976.0>,
             {dict,21,16,16,8,80,48,
                 {[],[],[],[],[],[],[],[],[],[],[],[],[],[],[],[]},
                 {{[[<0.9324.0>|#Ref<0.0.1.31485>]],
                   [[<0.8783.0>|#Ref<0.0.1.31479>],
                    [<0.9229.0>|#Ref<0.0.1.31483>]],
                   [],
                   [[<23061.2976.0>|{delegate_5,<23061.2976.0>}]],
                   [[<23061.9854.0>|{delegate_5,<23061.9854.0>}]],
                   [[<23061.3950.0>|{delegate_5,<23061.3950.0>}],
                    [<0.6189.0>|#Ref<0.0.0.91575>],
                    [<0.8771.0>|#Ref<0.0.1.31478>],
                    [<0.9281.0>|#Ref<0.0.1.31484>]],
                   [[<23061.11251.0>|{delegate_5,<23061.11251.0>}]],
                   [[<23062.3330.0>|{delegate_5,<23062.3330.0>}]],
                   [[<23062.3626.0>|{delegate_5,<23062.3626.0>}],
                    [<23062.3841.0>|{delegate_5,<23062.3841.0>}],
                    [<0.9220.0>|#Ref<0.0.1.31482>]],
                   [],
                   [[<0.8975.0>|#Ref<0.0.1.31480>]],
                   [[<23062.4331.0>|{delegate_5,<23062.4331.0>}]],
                   [],
                   [[<23061.4006.0>|{delegate_5,<23061.4006.0>}],
                    [<23062.2890.0>|{delegate_5,<23062.2890.0>}]],
                   [[<23061.3760.0>|{delegate_5,<23061.3760.0>}],
                    [<0.3385.0>|#Ref<0.0.0.28884>],
                    [<0.9043.0>|#Ref<0.0.1.31481>]],
                   []}}}],
            [{file,"src/pmon.erl"},{line,83}]},
        {lists,foldl,3,[{file,"lists.erl"},{line,1248}]},
        {rabbit_mirror_queue_slave,promote_me,2,
            [{file,"src/rabbit_mirror_queue_slave.erl"},{line,645}]},
        {rabbit_mirror_queue_slave,handle_call,3,
            [{file,"src/rabbit_mirror_queue_slave.erl"},{line,221}]},
        {gen_server2,handle_msg,2,[{file,"src/gen_server2.erl"},{line,1014}]},
        {proc_lib,wake_up,3,[{file,"proc_lib.erl"},{line,249}]}]}

Spec states that demonitor/2 should always return #state{}, but it
wasn't the case when pid wasn't found in #state.dict. This made API
unsafe to use, as it could lead to some other process storing incorrect
data as a pmon state.
@michaelklishin
Copy link
Member

@binarin should this go into stable?

@binarin
Copy link
Contributor Author

binarin commented Nov 19, 2015

I think so, but there is no 'stable' branch in this repo yet )

@michaelklishin
Copy link
Member

@binarin yeah, pmon is in rabbitmq-server in stable. Feel free to submit there as well.

@michaelklishin michaelklishin self-assigned this Nov 19, 2015
@michaelklishin michaelklishin added this to the 3.6.0 milestone Nov 19, 2015
michaelklishin added a commit that referenced this pull request Nov 19, 2015
@michaelklishin michaelklishin merged commit 58f6467 into rabbitmq:master Nov 19, 2015
@michaelklishin
Copy link
Member

@binarin thank you!

@videlalvaro
Copy link
Collaborator

@binarin this is a great catch! It's amazing how long this bug was there actually. Thanks for debugging this out!

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

Successfully merging this pull request may close these issues.

None yet

3 participants