[Swan-dev] Tried to address the comments of commit d7eb264

[D. Hugh Redelmeier]

| From: Sahana Prasad <sahana.prasad07 at gmail.com>

| Thank you for the comments.

Thanks for your code and thanks for your proposed changes.

History of VLA (variable-length arrays) in C.

- originally: all C types, when used for allocating memory, have to
  have a size that is known at compile time.  (This is true of array
  element types too.)  (That size must not be zero.)

- people felt the need of a way to allocate a variable-length thing on
  the stack (auto).  alloca(3) was the original horrible hack for this
  purpose.  The C Committee did not see how to standardize this
  cleanly.

- So VLAs were hacked into the C99 standard by the C Standards
  Committee (long after the original C standard).  I say hacked
  because this one simple feature had ramifications throughout the
  language.  Example: a sizeof expression is no longer always a
  compile-time constant.

- C++ did not and will not follow suit.  This is a problem for the C
  Committee since they think that it's the C Committee's mandate to
  keep C alligned with C++.

- So C11 made it an optional feature.  The C Committee seems to have
  regretted adding VLAs.

You (probably accidentally) used two VLAs.  A fairly natural thing to do.
But not really the C Way.

One common approach is to guess a constant bound and use that.
Often an upper bound, not an exact bound.  The risk is that the
bound may not be fundamental and overflow might happen.  Checking is
required to police this.  Here lie many serious security bugs.

Another approach is to use heap-allocation.  The problems are:
- heap allocation is expensive
- heap allocation is error-prone (leaks)
The size of a heap allocation might only be a guess.  The great thing
is that if you are about to overlow a heap allocation it is often
relatively easy to allocate more.

Your code changes replaced one VLA with an array with a constant
bound.  The bound is correct and organic.  That's good.

You replaced to other VLA with heap allocation.  Unfortunately you
didn't free it on all code paths.  This is a typical bug with heap
allocation.  This can be fixed but code maintenance could re-introduce
it very easily.

My favourite solution would be to figure out a fixed, modest,
proveable bound for the local array.  I pointed out that currently
there is only ever one value used as the bound and it is a
compile-time constant (unless I've misread the code).  What I don't
know is if that constant might become wrong as new features are
introduced to the code.  I often use passert to make sure such
assumptions are explicit and checked.

The final trick: you don't actually need a buffer.  You are using
in_raw to read bytes from a PBS and then comparing the bytes with a
previously-computed chunk.  Instead we could compare the chunk
directly with the bytes within the PBS.

- method one: just reach into the PBS and do a memeq

- method two (cleaner): add a chunk-comparing operator that looks a
  lot like in_raw.  One inspiration, from another domain, might be the
  "eat" macro.  Perhaps the result should be an err_t because there
  are multiple failure modes: at least "unequal" and "payload too
  small".

The second approach would more compelling if we could find other
places where it would be useful.  I have not looked.