[Emacs-bug-tracker] bug#6901: closed ([Patch]Memory Leak in chown-core.c)

[GNU bug Tracking System]

Your message dated Mon, 23 Aug 2010 23:03:47 -0600
with message-id <address@hidden>
and subject line Re: bug#6901: [Patch]Memory Leak in chown-core.c
has caused the GNU bug report #6901,
regarding [Patch]Memory Leak in chown-core.c
to be marked as done.

(If you believe you have received this mail in error, please contact
address@hidden)


-- 
6901: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=6901
GNU Bug Tracking System
Contact address@hidden with problems
> --- Begin Message ---
>
>
>
> Subject: 
>
> [Patch]Memory Leak in chown-core.c
>
>
>
>
> Date: 
>
> Mon, 23 Aug 2010 23:31:27 -0500
>
>
>
>
> 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.
>
> Is there any real momentum towards actually deprecating the verbose option on ch*? (I know we have been using ch???, but ch???? would look funny and unintuitive.)
>
> From 64f53a193c9c075e6461711a42d8df33ba01407a Mon Sep 17 00:00:00 2001
> From: Patrick W. Plusnick II <address@hidden>
> Date: Mon, 23 Aug 2010 13:32:20 -0500
> Subject: [PATCH] chown, chgrp: fixed a small, unlikely to occur memory leak
>
> *chown-core.c: fixed a small memory leak that would occur in describe-change.
> If it got to the default case and allocated memory to spec_alloc it would
> never deallocate it.
> ---
>  src/chown-core.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/src/chown-core.c b/src/chown-core.c
> index 1d3f74c..0e2bad4 100644
> --- a/src/chown-core.c
> +++ b/src/chown-core.c
> @@ -157,6 +157,7 @@ describe_change (const char *file, enum Change_status changed,
>               : _("ownership of %s retained\n"));
>        break;
>      default:
> +      free (spec_allocated);
>        abort ();
>      }
>  
> -- 
> 1.7.0.4
>
>
>
> -- 
> "Once GNU is written, everyone will be able to obtain good system software free, just like air."-- RMS in the GNU Manifesto
>
> --- End Message ---
> --- Begin Message ---
>
>
>
> Subject: 
>
> Re: bug#6901: [Patch]Memory Leak in chown-core.c
>
>
>
>
> Date: 
>
> Mon, 23 Aug 2010 23:03:47 -0600
>
>
>
>
> User-agent: 
>
> Mutt/1.5.18 (2008-05-17)
>
>
>
>
> 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
>
>
> --- End Message ---