1272955 - Crash in pl_Def* with Browsec VPN addon

Status: RESOLVED WORKSFORME

(NSPR :: NSPR, defect)

Description

[lovemetal_vgf]

Can you help me with this annoying crash please?

This bug was filed from the Socorro interface and is 
report bp-722c5678-da14-43a3-be5a-61d6a2160515.
=============================================================

Thank you.

Comment 1

[Loic]

Do you have steps to reproduce? Did you test in safe mode and with a fresh profile?

Comment 2

[Nicholas Nethercote [inactive]]

I see about 550 null deref crashes in the past 7 days in the following pl_Def* functions: pl_DefSend, pl_DefGetsocketoption, pl_DefGetsockname:

https://crash-stats.mozilla.com/search/?signature=~pl_Def&address=%3D0x0&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

They occur in versions between FF29 and FF49.

I took a look at the minidump for one report (https://crash-stats.mozilla.com/report/index/d1dfbe93-3007-4f95-8c67-313912160522). It said that |fd->lower| was nullptr, which is not supposed to happen -- there is a |PR_ASSERT(fd->lower != NULL);| immediately before the crash point.

Comment 3

[Nicholas Nethercote [inactive]]

Kai, any ideas?

Comment 4

[lovemetal_vgf]

UPDATE: I deactivated the add-on Browsec and no more crashes.

Comment 5

[Franziskus Kiefer [:franziskus]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> I see about 550 null deref crashes in the past 7 days in the following
> pl_Def* functions: pl_DefSend, pl_DefGetsocketoption, pl_DefGetsockname:
> 
> It said that |fd->lower| was nullptr, which is not supposed
> to happen -- there is a |PR_ASSERT(fd->lower != NULL);| immediately before
> the crash point.

PR_ASSERT has no effect in opt builds. All those functions (pl_Def*) have to check the arguments they're using for nullptr.

Comment 6

[Nicholas Nethercote [inactive]]

> PR_ASSERT has no effect in opt builds. All those functions (pl_Def*) have to
> check the arguments they're using for nullptr.

The PR_ASSERT suggests that a null fd->lower is expected to be impossible, which suggests that there's a bug elsewhere. So the fix is, to me, unclear. We could just skip the call if fd->lower is null and return something (not sure what), but maybe that's no better than crashing?

Comment 7

[Kai Engert (:KaiE:)]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> I took a look at the minidump for one report
> (https://crash-stats.mozilla.com/report/index/d1dfbe93-3007-4f95-8c67-
> 313912160522). It said that |fd->lower| was nullptr,

For my education, where on that page do you see that fd->lower was NULL ?

Comment 8

[Kai Engert (:KaiE:)]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> We could just skip the call if fd->lower is null and return something (not
> sure what), but maybe that's no better than crashing?

I think we shouldn't crash in this scenario. If fd->lower is null, I think we're already in a corrupted state and shouldn't proceed further.

If fd->lower is null, we have:
- either memory corruption
- or some sort of race, where a socket has already been closed/destroyed, but another thread is still operating on it.

Comment 9

[Nicholas Nethercote [inactive]]

> > I took a look at the minidump for one report
> > (https://crash-stats.mozilla.com/report/index/d1dfbe93-3007-4f95-8c67-
> > 313912160522). It said that |fd->lower| was nullptr,
> 
> For my education, where on that page do you see that fd->lower was NULL ?

It's not on that page. You have to download the minidump -- which requires minidump access privileges, which not everybody has -- and open it in MSVC. See https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/Debugging_a_minidump for more details.

Comment 10

[Marco Castelluccio [:marco]]

(100.0% in signature vs 00.30% overall) Addon "Browsec VPN" = true

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.