957458 - _PR_UNSET_THREAD_RWLOCK_RANK decrements |index| incorrectly in the while loop

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Patch

_PR_UNSET_THREAD_RWLOCK_RANK has a while loop:

	index = lock_stack->trs_index - 1;
	while (index-- >= 0) {
		if ((lock_stack->trs_stack[index] == rwlock) && !done)  {
			...

The intention of the code is to decrement |index| at the end of the loop
body, but the loop body actually uses the decremented value of |index|.

The patch changes the while loop to a for loop so that we can decrement
|index| at the right time. It also makes the following changes.

1. _PR_SET_THREAD_RWLOCK_RANK and _PR_UNSET_THREAD_RWLOCK_RANK are called
only when rwlock->rw_rank is not PR_RWLOCK_RANK_NONE. It is incorrect to
add a lock with PR_RWLOCK_RANK_NONE to the thread's lock stack.

2. In two if statements, perform the cheaper test before the more
expensive test.

Comment 1

[Wan-Teh Chang]

Attachment: Patch v1.1

Use TAB characters, following the prevalent style in the file.

Comment 2

[Kai Engert [:KaiE:]]

Comment on attachment 8356952 [details] [diff] [review]
Patch v1.1

r=kaie

Comment 3

[Wan-Teh Chang]

Attachment: Patch v2

I found that _PR_GET_THREAD_RWLOCK_RANK has a serious bug: it also needs
to return PR_RWLOCK_RANK_NONE when lock_stack exists but is empty
(lock_stack->trs_index == 0).

This bug implies either no NSPR users are using a PRRWLock with a nonzero
lock rank, or none of those NSPR users are using debug builds of NSPR.
So we can also consider just removing the lock rank checking code, and
changing PR_NewRWLock to assert that lock_rank == PR_RWLOCK_RANK_NONE.

I changed the patch to also fix this bug.

Comment 4

[Wan-Teh Chang]

Attachment: Add a rwlockrank.c test

Comment 5

[Wan-Teh Chang]

Attachment: Print a pointer using the %p format specifier

This patch fixes a printf format problem I noticed in rwlocktest.c.

Comment 6

[Wan-Teh Chang]

Comment on attachment 8357338 [details] [diff] [review]
Patch v2

Review of attachment 8357338 [details] [diff] [review]:
-----------------------------------------------------------------

I single-stepped the relevant code in the debugger and verified it
does the right thing now.

I think I can implement the lock stack in a more efficient way, but
since this is just debugging code and one of the bugs implies
nobody has ever used it, I don't want to spend more time on this.

Comment 7

[Kai Engert [:KaiE:]]

Comment on attachment 8357338 [details] [diff] [review]
Patch v2

r=kaie

You might consider to write the code in a simpler way, to make it more readable, and I'm always worried by statements that combine comparisons and assignements (your statement is right, but it requires more time to read such statements to convince oneself it is correct).

Instead of:
  if ((lock_stack = PR_GetThreadPrivate(pr_thread_rwlock_key)) == NULL ||
      lock_stack->trs_index == 0)

I'd use:
  lock_stack = PR_GetThreadPrivate(pr_thread_rwlock_key);
  if (!lock_stack || !lock_stack->trs_index)

Comment 8

[Kai Engert [:KaiE:]]

Comment on attachment 8357340 [details] [diff] [review]
Add a rwlockrank.c test

Why do you destroy only one of the locks?

Comment 9

[Wan-Teh Chang]

Attachment: Patch v3

Kai, I made the change you suggested. Thanks!

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/88cdc3e565e5

Comment 10

[Wan-Teh Chang]

Comment on attachment 8357348 [details] [diff] [review]
Print a pointer using the %p format specifier

Review of attachment 8357348 [details] [diff] [review]:
-----------------------------------------------------------------

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/b6674f2a3776

Comment 11

[Wan-Teh Chang]

Attachment: Add a rwlockrank.c test, v2

Kai, thank you for catching the missing PR_DestroyRWLock calls.

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/518d6d014628