1223909 - Atomic32::operator += and -= have non-canonical implementation

Status: RESOLVED INCOMPLETE

(Core :: WebRTC, defect, P3)

Description

[q1]

The Windows versions of Atomic32::operator += and operator -= (media\webrtc\trunk\webrtc\system_wrappers\source\atomic32_win.cc) have non-canonical implementations. They return the per-operation value, not the post-operation value. This implementation is also at odds with the Unix and Mac implementations of the same class, which return the post-operation value (see atomic32_posix.cc and atomic32_mac.cc).

If these functions are used for, e.g., reference counting, this error could cause a security bug, hence I've filed it that way.


Details
-------

41: int32_t Atomic32::operator+=(int32_t value) {
42:   return InterlockedExchangeAdd(reinterpret_cast<volatile LONG*>(&value_),
43:                                 value);
44: }
45: 
46: int32_t Atomic32::operator-=(int32_t value) {
47:   return InterlockedExchangeAdd(reinterpret_cast<volatile LONG*>(&value_),
48:                                 -value);
49: }

https://msdn.microsoft.com/en-us/library/windows/desktop/ms683597%28v=vs.85%29.aspx :

------
LONG __cdecl InterlockedExchangeAdd(
  _Inout_ LONG volatile *Addend,
  _In_    LONG          Value
);

...
The function returns the initial value of the Addend parameter.
------

Comment 1

[q1]

> per-operation value

should read

> pre-operation value

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Note: upstream bug.

As best I can tell from auditing all the uses of Atomic32 (using m-c), none of them use += or -=.  They all use ++ and --.  So while this might the a latent problem, there appears to be no actual issue here.

Comment 3

[q1]

(In reply to Randell Jesup [:jesup] from comment #2)
> Note: upstream bug.
> 
> As best I can tell from auditing all the uses of Atomic32 (using m-c), none
> of them use += or -=.  They all use ++ and --.  So while this might the a
> latent problem, there appears to be no actual issue here.

+= is used by Atomic32::Value() (atomic32.h) at least, although that case uses the expression |*this += 0|, so this bug doesn't change the result. Oddly, DXR finds that call only for the Posix atomic32.cc implementation, even though it exists in the .h file. DXR doesn't find any callers for -=, FWIW.

Comment 4

[q1]

> using m-c

What's that tool?

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

(In reply to q1 from comment #4)
> > using m-c
> 
> What's that tool?

m-c is short for mozilla-central.  I grepped all uses of Atomic32, then looked at all uses of those variables in the code.

Simplest would be to delete the impls and see if it still links.

Comment 6

[q1]

> Simplest would be to delete the impls and see if it still links.

The link succeeds on -=, but fails on undefined += via the call in Atomic32::Value(). If you replace the contents of Value() with |return 0;|, it links, so that's the only current use, so the bug is latent.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Mass change P3->P4 to align with new Mozilla triage process.

Comment 8

[Dan Minor [:dminor]]

This code has been rewritten since the bug was filed.