ChatSecureAndroid icon indicating copy to clipboard operation
ChatSecureAndroid copied to clipboard

potential data races in NewChatActivity.java

Open yulin2 opened this issue 10 years ago • 2 comments

Dear developers of ChatSecureAndroid,

I'm a Ph.D. student and I'm doing research on checking data race for Android apps. I found there are data races that may lead to potential bugs in NewChatActivity.java:

The AsyncTask in "startGroupChat" method writes to field "mRequestedChatId" at line 2173. This task is eventually be called by "onLoadFinished" method at line 300 through method "resolveIntent". However, "onLoadFinished" also uses "mRequestedChatId" at lines 301-303. This lead to data race on "mRequestedChatId". Do you think this race may lead to potential bugs? Do you need synchronization on "mRequestedChatId"?

"mRequestedChatId" is also written by "startChat" method at line 1944 and 1950, this can also be a race with the written at line 2173.

Also, the AsyncTask invokes "showChat" method and "showChat" access the GUI elements "mChatPagerAdapter" and "mChatPager" at line 690 and 700. Android documents say we shouldn't access GUI outside main thread. So this should be a problem. It also leads to a race on these GUI widgets and the Cursor at line 690, since "mChatPagerAdapter", "mChatPager" and the Cursor is also read/written by main thread. A fix could be put the statements at 690 and 700 into a Handler or runOnUiThread method.

Thanks, Yu

yulin2 avatar Feb 13 '14 04:02 yulin2

Dear developers,

I reported this issue two weeks ago. Do you have any comments on these races? Will they lead to concurrency bugs? It'd be better if we can eliminate these races.

Thanks, Yu

yulin2 avatar Mar 04 '14 23:03 yulin2

We are reworking NewChatActivity right now, and will take all of this important feedback into consideration.

n8fr8 avatar Mar 06 '14 16:03 n8fr8