1008792 - Null pointer deref on out of memory in nr_socket_buffered_stun_create

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

if (!(sock = RCALLOC(sizeof(nr_socket_buffered_stun))))
    ABORT(R_NO_MEMORY);

  sock->inner = inner;

...

  if (_status) {
    void *sock_v = sock;
    sock->inner = 0;  /* Give up ownership so we don't destroy */
    nr_socket_buffered_stun_destroy(&sock_v);
  }

We need if (sock)  around sock->inner

Comment 1

[Eric Rescorla (:ekr)]

Credit to Sylvestre Ledru for static analysis that caught this.

Comment 2

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

This is a potential null-deref in OOM, that doesn't appear to be a sec issue to me.

Comment 3

[Michael Froman [:mjf]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e04d846dcfd0

Comment 4

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1008792: Check for valid pointer before using.

Bug 1008792: Check for valid pointer before using.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8655089 [details]
MozReview Request: Bug 1008792: Check for valid pointer before using.

https://reviewboard.mozilla.org/r/17815/#review15907

Comment 6

[Eric Rescorla (:ekr)]

https://reviewboard.mozilla.org/r/17815/#review16021

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c:168
(Diff revision 1)
>    if (_status) {
>      void *sock_v = sock;
> +    if (sock) {
> -    sock->inner = 0;  /* Give up ownership so we don't destroy */
> +      sock->inner = 0;  /* Give up ownership so we don't destroy */
> +    }
>      nr_socket_buffered_stun_destroy(&sock_v);
>    }

This is an odd change. Why are you calling nr_socket_buffered_stun_destroy() on a pointer you know points to NULL? It's safe, but silly.

You should put the entire destructor clause inside the if (sock).

Comment 7

[Eric Rescorla (:ekr)]

removed checkin-needed because of c6

Comment 8

[Michael Froman [:mjf]]

Comment on attachment 8655089 [details]
MozReview Request: Bug 1008792: Check for valid pointer before using.

Bug 1008792: Check for valid pointer before using.

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8655089 [details]
MozReview Request: Bug 1008792: Check for valid pointer before using.

https://reviewboard.mozilla.org/r/17815/#review16033

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b150362b0f4

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7b150362b0f4