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

potential data races in RouteselectActivity.java #10

Closed
yulin2 opened this Issue Feb 14, 2014 · 8 comments

Comments

Projects
None yet
3 participants
@yulin2

yulin2 commented Feb 14, 2014

Dear developers of grtransit,

I'm a Ph.D. student and I'm doing research on checking data races for Android apps. I found there is a data race in RouteselectActivity.java:

At line 158, the cursor "mCsr" is written in the AsyncTask (and this is the only written for "mCsr"). However, "mCsr" is read by "onListItemClick" at line 59 and 60. The order of the written and read is not guaranteed: after the "onCreate()" finishes, user may touch the items on the screen (i.e., "onListItemClick()" is invoked) and "mCsr" is read. But at this time point, the line 158 in AsyncTask may not be executed or not finish yet, because of the non-deterministic of concurrency. If so, there will be a NullPointerException at line 59.

A quick fix can be add a null checking before line 59:
if(mCsr == null) return;

A more complex fix would be using wait/notify, or latch.

What do you think?

Thanks,
Yu

@gdmalet

This comment has been minimized.

Show comment
Hide comment
@gdmalet

gdmalet Feb 14, 2014

Owner

Hi Yu,

Many thanks for the feedback. I shall look into when I get a moment --
probably this weekend.

gdm

Owner

gdmalet commented Feb 14, 2014

Hi Yu,

Many thanks for the feedback. I shall look into when I get a moment --
probably this weekend.

gdm

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Feb 16, 2014

Great. I'm looking forward to your opinion.

Yu

yulin2 commented Feb 16, 2014

Great. I'm looking forward to your opinion.

Yu

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Feb 23, 2014

Hi gdm,

Do you get a chance to look at this issue?

Yu

yulin2 commented Feb 23, 2014

Hi gdm,

Do you get a chance to look at this issue?

Yu

@gdmalet

This comment has been minimized.

Show comment
Hide comment
@gdmalet

gdmalet Feb 24, 2014

Owner

On 02/23/2014 10:57 AM, yulin2 wrote:

Do you get a chance to look at this issue?

Yes, sorry, I've been a bit slow getting back to you.

I agree there's definitely a problem there, as mCsr could indeed be used
before it is initialised. I haven't fixed the code yet, but think I will
probably at least initially do as you first suggested, and ignore a
touch if we're not ready. This is not ideal but does avoid a crash. I do
need to do a proper fix sometime though.

Many thanks for pointing this out. Fortunately this is not much of a
problem in practice, as I can see from exception reports provided by
Google. If it had been more of a problem I would have caught it quicker,
so the bug sits in that sorry place of annoying a few people
occasionally.....

gdm

Owner

gdmalet commented Feb 24, 2014

On 02/23/2014 10:57 AM, yulin2 wrote:

Do you get a chance to look at this issue?

Yes, sorry, I've been a bit slow getting back to you.

I agree there's definitely a problem there, as mCsr could indeed be used
before it is initialised. I haven't fixed the code yet, but think I will
probably at least initially do as you first suggested, and ignore a
touch if we're not ready. This is not ideal but does avoid a crash. I do
need to do a proper fix sometime though.

Many thanks for pointing this out. Fortunately this is not much of a
problem in practice, as I can see from exception reports provided by
Google. If it had been more of a problem I would have caught it quicker,
so the bug sits in that sorry place of annoying a few people
occasionally.....

gdm

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Feb 24, 2014

I agree this kind of bug has a very low chance to occur in practice, since the buggy interleaving is so tricky. But we still need to be careful and avoid harmful races when we move some code into AsyncTask or Thread.

Thanks for your opinion,
Yu

yulin2 commented Feb 24, 2014

I agree this kind of bug has a very low chance to occur in practice, since the buggy interleaving is so tricky. But we still need to be careful and avoid harmful races when we move some code into AsyncTask or Thread.

Thanks for your opinion,
Yu

@burcuku

This comment has been minimized.

Show comment
Hide comment
@burcuku

burcuku Nov 9, 2015

Hi,

I am a PhD student working on concurrency in Android apps. While examining the potential race yulin2 has pointed out, I realized that the accesses to mCsr are implicitly ordered and do not result in a real race.

onListItemClick can be triggered only after the list adapter is set and the items are visible. The list adapter is set in onPostExecute method, after the completion of the task in doInBackground. Hence, mCsr is already set when onListItemClick is executed.

I would like to get your comments on this issue.

Thanks in advance.

burcuku commented Nov 9, 2015

Hi,

I am a PhD student working on concurrency in Android apps. While examining the potential race yulin2 has pointed out, I realized that the accesses to mCsr are implicitly ordered and do not result in a real race.

onListItemClick can be triggered only after the list adapter is set and the items are visible. The list adapter is set in onPostExecute method, after the completion of the task in doInBackground. Hence, mCsr is already set when onListItemClick is executed.

I would like to get your comments on this issue.

Thanks in advance.

@gdmalet

This comment has been minimized.

Show comment
Hide comment
@gdmalet

gdmalet Nov 10, 2015

Owner

@burcuku, thanks for the comments! I'm going to be working on the code in the next few days, and will study what you said and give a proper answer. I'm working on something else at the moment and don't want to be distracted :-)

Owner

gdmalet commented Nov 10, 2015

@burcuku, thanks for the comments! I'm going to be working on the code in the next few days, and will study what you said and give a proper answer. I'm working on something else at the moment and don't want to be distracted :-)

@gdmalet

This comment has been minimized.

Show comment
Hide comment
@gdmalet

gdmalet Nov 24, 2015

Owner

@burcuku I think @yulin2 was looking at a version of the code closer to https://github.com/gdmalet/grtransit/blob/871ca639ed547efbf85f821811a3bfb43c08816e/Android/app/src/main/java/net/kw/shrdlu/grtgtfs/Activities/RouteselectActivity.java .

The line numbers don't quite match up to his comments, but lines 73 and 74 do indeed use mCsr. However, I agree with your comments that the touch-listener is not set until after mCsr is set (line 206 in that version), so I don't think there is or was a problem.

This was fixed in changed anyhow in commit 11be118 -- see
https://github.com/gdmalet/grtransit/blob/11be11851f876198745edb707eb9c811893da48f/Android/app/src/main/java/net/kw/shrdlu/grtgtfs/Activities/RouteselectActivity.java . In that version in line 78 and on the values that were stored in the textView are read instead, which is much safer, as it doesn't need the cursor at all.

I'm going to close this issue as I think we agree there is no problem. Many thanks for your input though! I appreciate people taking the time to check my work, although I must admit I work on this app in irregular bursts so can be slow to respond, so apologize for that.

g

Owner

gdmalet commented Nov 24, 2015

@burcuku I think @yulin2 was looking at a version of the code closer to https://github.com/gdmalet/grtransit/blob/871ca639ed547efbf85f821811a3bfb43c08816e/Android/app/src/main/java/net/kw/shrdlu/grtgtfs/Activities/RouteselectActivity.java .

The line numbers don't quite match up to his comments, but lines 73 and 74 do indeed use mCsr. However, I agree with your comments that the touch-listener is not set until after mCsr is set (line 206 in that version), so I don't think there is or was a problem.

This was fixed in changed anyhow in commit 11be118 -- see
https://github.com/gdmalet/grtransit/blob/11be11851f876198745edb707eb9c811893da48f/Android/app/src/main/java/net/kw/shrdlu/grtgtfs/Activities/RouteselectActivity.java . In that version in line 78 and on the values that were stored in the textView are read instead, which is much safer, as it doesn't need the cursor at all.

I'm going to close this issue as I think we agree there is no problem. Many thanks for your input though! I appreciate people taking the time to check my work, although I must admit I work on this app in irregular bursts so can be slow to respond, so apologize for that.

g

@gdmalet gdmalet closed this Nov 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment