Re: (ITS#5340) REP_ENTRY_MODIFIABLE bug in dynlist

[h.b.furuseth@usit.uio.no]

[Copying some discussion from IRC, and adding some more]

I'm a bit nervous about doing more than minimal fixes to this until we
do something major, because it's so broken.  Fix one bit and maybe
something will pop up another place - i.e. a regression.

Also following existing practice can mean fix one bug into a different
bug, which Ando recently did for ITS#6423.  (dynlist resets MUSTRELEASE
but doesn't obey it, rwm works around a similar bug in rwm or dynlist.)
Though he got other fixes right.  Anyway:

To repeat, the main problems with the current code:

* Code does not always clean up properly, leaving e.g. flags behind.

* Code which does clean up sometimes does it halfway, e.g. it resets
  MUSTBEFREED/MUSTRELEASE but not MODIFIABLE.  So, no wonder there
  are unwanted MODIFIABLE flags hanging about (ITS#6423).
  Or it dup()s an entry but does not release the old one.
  
* op_*() generally expects SlapReply to be preinitialized.  That
  worked before overlays, but now modules (backends, overlays)
  inherit values that the previous module did not clean up during
  an operation.

  Howard suggested that we should restore that - code which puts
  something into a SlapReply should ensure it's taken out again.

  I've started on that in the enclosed patch, but on second thought
  I think that's not the right fix because it's the most fragile
  solution: Practically every module which depends on it can break
  if some other module gets it wrong.

  OTOH, modules can't just reset SlapReply since some of it is input
  values to op_*():

  - Controls can be set in backend_check_restrictions() before the call
    to op_*().

  - dynlist.c sets r.sr_attr_flags before calling o.o_bd->be_search(), I
    haven't looked at what effect that has.

I'm thinking modules should instead initialize SlapReply.sr_u and maybe
some sr_flags themselves instead of depending on preinit, but I don't
know when I'll get back to doing something about that.  Xmas coming...

So anyway, here is a quick patch which actually tries to be rather
conservative, sans a bunch of RS_ASSERT() lines which can be safely
deleted in real code.  It only addresses sr_entry and sporadically
attrs, not sr_refs/ctrls.  It passes make test for me.  Please review.

Where updates to flags are broken and needs fixing, I change to use
the new function rs_ensure_entry_modifiable() etc, but I leave correct
updates alone - as well as some updates where I don't know which overlay
should be passed to these function.

Description of changes:

All over the place:
   Clear MUSTBEFREED/MUSTRELEASE by clearing all of REP_ENTRY_MASK,
   not just the particular flag.
   Reset attrs, entry, flags a number of places.  (Not very
   consistently, sorry.)

   Lots of RS_ASSERT()s for debugging.  Only defined when LDAP_DEVEL.

proto-slap.h, result.c: Move RS_ASSERT(), add rs_flush_entry().

result.c:
    MODIFIABLE is independent of MUSTBEFREED - the modifiable entry can
    e.g. be on the stack.

result.c, extended.c:
    When changing sr->sr_type to extended/sasl, flush sr_un (usually the
    entry) first, since another member of the union will be used.

    For this reason, updates to sr_type are "suspicious" code to be avoided,
    so some code elsewhere is changed to initialize it to REP_RESULT rather
    than setting it to that after initialization.

bconfig.c, servers/slapd/syncrepl.c:
    Avoid reusing SlapReply a number of places where that's easy.

slap.h:
    Cast the REP_... flags to slap_mask_t, so ~REP_... will work right.

back-bdb/search.c:
    Delay setting rs->sr_entry until we know it'll be used.
    (IIRC made a difference around a paged results response.)

back-shell/result.c, back-sock/result.c:
    Set MUSTBEFREED before sending, obey it afterwards.

overlays/constraint.c, overlays/pcache.c:
    Drop nulling of fields that were just nulled by initialization.

overlays/dynlist.c:
    When "stealing" an entry from SlapReply, grab and reset the flags
    that go with it, so it can be properly freed/released.  Restore
    the flags and entry later, then obey the flags (release/free).
    Do this unconditionally, it happens only once per SlapReply anyway.

overlays/pcache.c:
    Obey and reset entry flags properly.

    Remove unused SlapReply parameters to some functions - did this
    for readibility while trying to figure out what was going on.

overlays/sssvlv.c:
    Since setting MUSTRELEASE before sending, obey it afterwards.

overlays/translucent.c:
    Reset flags properly too when releasing/freeing entry.
    Streamline translucent_search() loop to clarify ownership of entry.

overlays/rwm.c, servers/slapd/slapi/slapi_pblock.c:
    Obey flags properly.