[PATCH v2 0/7] trace2: stop allowing die()

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

After v1 was posted, based on a concrete example of tracing leading to a
recursive die() problem, more evidence has come up to imply that allocations
are failing for some users more often. This is potentially an issue with the
allocator chosen by Git for Windows, which is being discussed elsewhere.

But the conclusion is this: the trace2 API shouldn't call helpers that might
call die(). It's too low-level for that.

In this v2, I have a much more robust approach to removing die() from the
trace2 API.

This starts with a new banned-die.h header file at the root of the repo and
including it from all trace2 API *.c files. It starts empty, but the later
patches will add one method at a time:

 * xsnprintf() : This is the original patch, but made more complete by
   adding the method to banned-die.h.
 * xstrdup()
 * ALLOC_ARRAY()
 * xstrfmt()
 * ALLOC_GROW()
 * xcalloc()

During each patch, the goal was to have the trace2 logic be "as correct as
possible" when an allocation failure occurs. This may mean that we have
incomplete messages or dropped trace messages.

The focus here is that the trace2 API should never cause a process-ending
failure, because those failures will trigger trace2 API calls while
reporting the failure.

Thanks, -Stolee

Derrick Stolee (7):
  banned-die: create header for banning of functions
  trace2: tolerate failed timestamp formatting
  trace2: remove use of xstrdup()
  trace2: remove use of ALLOC_ARRAY()
  trace2: remove use of xstrfmt()
  trace2: remove use of ALLOC_GROW()
  trace2: remove use of xcalloc()

 banned-die.h            | 32 +++++++++++++++++
 trace2.c                | 51 ++++++++++++++++++++++++---
 trace2/tr2_cfg.c        |  1 +
 trace2/tr2_cmd_name.c   |  1 +
 trace2/tr2_ctr.c        | 11 +++++-
 trace2/tr2_dst.c        |  1 +
 trace2/tr2_sid.c        |  1 +
 trace2/tr2_sysenv.c     |  7 ++--
 trace2/tr2_tbuf.c       | 50 +++++++++++++++++++--------
 trace2/tr2_tgt_event.c  |  1 +
 trace2/tr2_tgt_normal.c |  1 +
 trace2/tr2_tgt_perf.c   |  1 +
 trace2/tr2_tls.c        | 76 +++++++++++++++++++++++++++++++++++++++--
 trace2/tr2_tls.h        |  7 ++++
 trace2/tr2_tmr.c        | 15 ++++++--
 15 files changed, 229 insertions(+), 27 deletions(-)
 create mode 100644 banned-die.h


base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2178%2Fderrickstolee%2Ftrace2-dont-die-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2178/derrickstolee/trace2-dont-die-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2178

Range-diff vs v1:

 -:  ---------- > 1:  84634717e2 banned-die: create header for banning of functions
 1:  95c546bb3b ! 2:  bd45f46a34 trace2: tolerate failed timestamp formatting
     @@ Commit message
          triggering this problem in a loop as the 'atexit' event would be
          retriggered by the die().
      
     -    I could not determine the exact cause of why these errors started
     -    occuring in a bunch. My best guess is that these users are dogfooding an
     -    early operating system version that is more likely to fail in the
     -    gettimeofday() function and thus leaves the structures uninitialized and
     -    potentially violating the expected values.
     +    Based on other symptoms impacting users on the version reporting these
     +    failures, it is most likely that this is actually a failure to allocate
     +    memory, which is a specific symptom in Git for Windows. That fork uses a
     +    different library for its implementation of vsprintf() which allocates
     +    an array when seven or more positional arguments exist in the formatting
     +    string, such as this one.
      
     -    However, for full defense-in-depth I made several modifications:
     +    Ultimately, the trace2 machinery is so low-level that it should not rely on
     +    any helper functions that perform error handling with die(), as that can
     +    trigger issues that would then be traced, causing this kind of recursive
     +    loop.
     +
     +    These changes help remove any use of die() within this file:
      
          1. Both 'tv' and 'tm' structs are initialized with zero values, allowing
             an erroring gettimeofday() or gmtime_r() method to leave them
     @@ Commit message
          but they only die() on out-of-memory errors instead of formatting
          issues. I chose to leave those in place for now.
      
     +    Helped-by: Taylor Blau <ttaylorr@openai.com>
          Signed-off-by: Derrick Stolee <stolee@gmail.com>
      
     + ## banned-die.h ##
     +@@
     + #undef die
     + #define die banned(die)
     + 
     ++#undef xsnprintf
     ++#define xsnprintf(...) BANNED(xsnprintf)
     ++
     + #endif /* BANNED_DIE_H */
     +
       ## trace2/tr2_tbuf.c ##
      @@
       
 -:  ---------- > 3:  ec447a6a77 trace2: remove use of xstrdup()
 -:  ---------- > 4:  db6858d381 trace2: remove use of ALLOC_ARRAY()
 -:  ---------- > 5:  7f0bb405ad trace2: remove use of xstrfmt()
 -:  ---------- > 6:  120cf1967b trace2: remove use of ALLOC_GROW()
 -:  ---------- > 7:  c8fc195a2a trace2: remove use of xcalloc()

-- 
gitgitgadget