bug#6901: [Patch]Memory Leak in chown-core.c

[Bob Proulx]

William Plusnick wrote:
> There was a small memory leak in chown-core.c in the describe_change
> function that would occur in cases where memory was allocated to
> spec_allocated, but is passed an invalid Change_status. In that case it gets
> to the default case and simply aborted without ever deallocating the memory.
> ...
> @@ -157,6 +157,7 @@ describe_change (const char *file, enum Change_status 
> changed,
>               : _("ownership of %s retained\n"));
>        break;
>      default:
> +      free (spec_allocated);
>        abort ();
>      }

Thank you for your patch.  But I think you are not understanding what
is happening at that point in the code.  The 'changed' variable is of
type 'enum Change_status' which as presently defined has four possible
values.  In the code all four of those possibilities are tested.
There is no memory leak in any of those four possible cases.  The
allocated memory is freed up at the end of the routine.

But, the cautious programmer wonders, what happens if the definition
is changed and the two sections of code become out of sync with each
other?  What happens if an error is introduced into the code such that
through an internal coding error it isn't one of the four expected
possibilities?  They say to themselves, I know, I will code in a
default case with an abort() call.  If the impossible happens, if a
coding error is encountered, then the abort() will dump core alerting
the developer to the problem.  With an adequate set of test cases this
will be caught while testing the code and not by a user of the code.
It would be functionally correct to remove that 'default' case with
the abort() call.  It isn't required to be there.  It is extra to help
with debugging a potential but not occurring error.  That is what you
are seeing in the above.

Additionally the abort() is very similar to exit() in that the program
is not going to be running after that point.  There is no need to free
up allocated memory before an exit() call nor before an abort() call.
The program is going to stop running and the operating system will
free up all of the allocated memory.  It is not a memory leak.  The
abort() call is not a normal program exit.  It is an error exit
indicating a program bug has been found.  It should not be possible to
encounter that through normal program operation.  Here it is a safety
handler for that just-in-case event of an internal coding error.

Also changing the program data before the abort() call is undesirable.
The core dump wouldn't be in the same state as the problem that it is
trying to capture.  The abort() will dump the program state into a
core file so that the developer can analyze it.  In order to be useful
it should be an unmodified dump of the program state.

Bob