39110 - PR_FREEIF macro is dangerous

Status: NEW

(NSPR :: NSPR, defect, P3)

Description

[Erik van der Poel]

The PR_FREEIF macro is dangerous. If it is followed by "else", it will give an
unexpected result:

  if (foo)
    PR_FREEIF(ptr);
  else
    blah();

I suggest putting PR_BEGIN_MACRO and PR_END_MACRO around the relevant macros.

Comment 1

[Wan-Teh Chang]

The nested if-else problem.

The fix you suggested is the right one.

Comment 2

[Wan-Teh Chang]

In fact the sample code:
  if (foo)
    PR_FREEIF(ptr);
  else
    blah();
won't even compile because of the way
PR_DELETE is defined.  (PR_FREEIF uses
PR_DELETE.)  So PR_DELETE also needs
to be fixed.

I'm going to attach a patch that changes
PR_DELETE and PR_FREEIF to use PR_BEGIN_MACRO
and PR_END_MACRO.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch.

Comment 4

[Wan-Teh Chang]

The patch is checked in on the main trunk.
/cvsroot/mozilla/nsprpub/pr/include/prmem.h, revision 3.7

Added a new test freeif.c which contains Erik's
sample code.

Comment 5

[Wan-Teh Chang]

It was pointed out to me that some code
is depending on the current broken definition
of PR_DELETE and PR_FREEIF.  For example,
some files contain instances of PR_FREEIF(ptr)
that don't end with ';'.

I'm going to defer this change to the next
major release, NSPR 5.0, because it breaks
backward compatibility at source level.

Backed out the checkin.
/cvsroot/mozilla/nsprpub/pr/include/prmem.h, revision 3.8

The test freeif.c is removed from the makefile and test
harness.

Comment 6

[Thomas]

Are PR_DELETE and PR_FREEIF still broken?
As comment #5 seems to indicate that his bug is dependant on these...

Comment 7

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

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.