search

shadowproject / shadow

ShadowCore integration/staging tree
MIT License
95 stars 60 forks source link

SecureMessaging - bug 100%cpu - test fix #36

Closed kewde closed 8 years ago

kewde commented 8 years ago

I didn't test this bug fix just yet but I'm pretty sure that this was the issue at hand.. Will need someone to run this for long period of time to verify. @dasource

Deadlock LOCK(pnode->cs_vSend) -> PushMessage(..., pnode) -> BeginMessage(...) -> ENTER_CRITICAL_SECTION(cs_vSend)

https://github.com/shadowproject/shadow/blob/master/src/net.h#L531

ENTER_CRITICAL_SECTION is defined by MSDN as

Waits for ownership of the specified critical section object. The function returns when the calling thread is granted ownership.

I looked through the other code and found no instance where cs_vSend is locked before PushMessage. Instances verifying my suspicion: https://github.com/shadowproject/shadow/blob/f3fa333f837768823afe16c0347272b5cb8d6c2a/src/main.cpp#L341

kewde commented 8 years ago

Do not merge just yet, doing a small rewrite of SMSG.

rynomster commented 8 years ago

https://github.com/shadowproject/shadow/pull/36/commits/a1725918947bc95482aa6a46efda8169a4e43ec8 doesn't compile, but it also doesn't make sense.. I think we should change the add contact modal, because currently it changes the label as soon as you set it. We should probably only set the label and pubkey once the conversation starts.

kewde commented 8 years ago

If the label was not set, and a label was passed only then it would update.

I made it such that add_contact modal and open_conversation modal don't update the label if it already has one.

add_contact modal would scream 'duplicate address' yet somehow it managed to trigger appendAddresses with the new label. The label wasn't updated in reality tho. But a new contact appears in the addressbook list of the chat sidebar.

open_conversation I made it such that onInput address -> grab label if it exists and show in textbox, and basically render it unchangeable if it was already set.

I would all make handling the contact list a pain..

rynomster commented 8 years ago

updateLabel would trigger appendAddress from the backend. With the original modal, that was how the address was created :) so that as they type it would add it or change it in the address book.. but I've gotten rid of that now, no more updateAddressLabel from getPubKey, then in new conversation it creates the address now. I'm still working on it, but I will close this PR when I commit

kewde commented 8 years ago

Good idea.

I was in a hurry so I just thought let me rig getPubKey quickly but I too thought it was kind of useless but I was unsure if anything else relied on it.