Re: [RFA] Ensure result of make_cleanup is never NULL.

Doug Evans
2012-04-19T19:18:27Z

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Wed, Apr 18, 2012 at 7:36 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> Well, aren't all places using the sentinel, using it throught the macro?
> Pedro> What does this make more robust?
>
> If you initialize the object it can end up in a read-only section, so
> writes to it will SEGV.
>
> In this particular case I don't see that this would help a lot, since
> the object is just a sentinel.  But, it wouldn't hurt and maybe it would
> catch some really unusual bug.

Hi. fyi, here is what I committed.

I changed the name cleanup_sentinel -> sentinel_cleanup because I went
to print the value in gdb and spelled it the latter way and when that
failed I cursed because I was darn sure I had spelled it correctly.
That told me I liked the latter spelling better. :-) Anyways ...

2012-04-19 Doug Evans <dje@google.com>

* cleanups.h (struct cleanup): Move to cleanups.c.
(make_cleanup_dtor_ftype): New typedef.
(make_cleanup_dtor): Use it.
(ALL_CLEANUPS): Replace with ...
(all_cleanups): ... this. Declare. All uses updated.
* cleanups.c: #include "gdb_assert.h".
(sentinel_cleanup): New static global.
(SENTINEL_CLEANUP): Define.
(cleanup_chain, final_cleanup_chain): Initialize to SENTINEL_CLEANUP.
(make_my_cleanup2): Assert result is non-NULL.
(all_cleanups): New function.
(save_my_cleanups): Initialize new chain to SENTINEL_CLEANUP instead
of NULL.

[-- Attachment #2: gdb-120419-cleanups-3b.patch.txt --]
[-- Type: text/plain, Size: 8427 bytes --]

2012-04-19 Doug Evans <dje@google.com>

* cleanups.h (struct cleanup): Move to cleanups.c.
(make_cleanup_dtor_ftype): New typedef.
(make_cleanup_dtor): Use it.
(ALL_CLEANUPS): Replace with ...
(all_cleanups): ... this. Declare. All uses updated.
* cleanups.c: #include "gdb_assert.h".
(sentinel_cleanup): New static global.
(SENTINEL_CLEANUP): Define.
(cleanup_chain, final_cleanup_chain): Initialize to SENTINEL_CLEANUP.
(make_my_cleanup2): Assert result is non-NULL.
(all_cleanups): New function.
(save_my_cleanups): Initialize new chain to SENTINEL_CLEANUP instead
of NULL.

Index: cleanups.c
===================================================================
RCS file: /cvs/src/src/gdb/cleanups.c,v
retrieving revision 1.2
diff -u -p -r1.2 cleanups.c
--- cleanups.c 17 Apr 2012 21:24:47 -0000 1.2
+++ cleanups.c 19 Apr 2012 17:52:44 -0000
@@ -18,15 +18,53 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */

#include "defs.h"
+#include "gdb_assert.h"
+
+/* The cleanup list records things that have to be undone
+ if an error happens (descriptors to be closed, memory to be freed, etc.)
+ Each link in the chain records a function to call and an
+ argument to give it.
+
+ Use make_cleanup to add an element to the cleanup chain.
+ Use do_cleanups to do all cleanup actions back to a given
+ point in the chain. Use discard_cleanups to remove cleanups
+ from the chain back to a given point, not doing them.
+
+ If the argument is pointer to allocated memory, then you need
+ to additionally set the 'free_arg' member to a function that will
+ free that memory. This function will be called both when the cleanup
+ is executed and when it's discarded. */
+
+struct cleanup
+{
+ struct cleanup *next;
+ void (*function) (void *);
+ void (*free_arg) (void *);
+ void *arg;
+};
+
+/* Used to mark the end of a cleanup chain.
+ The value is chosen so that it:
+ - is non-NULL so that make_cleanup never returns NULL,
+ - causes a segv if dereferenced
+ [though this won't catch errors that a value of, say,
+ ((struct cleanup *) -1) will]
+ - displays as something useful when printed in gdb.
+ This is const for a bit of extra robustness.
+ It is initialized to coax gcc into putting it into .rodata.
+ All fields are initialized to survive -Wextra. */
+static const struct cleanup sentinel_cleanup = { 0, 0, 0, 0 };
+
+/* Handy macro to use when referring to sentinel_cleanup. */
+#define SENTINEL_CLEANUP ((struct cleanup *) &sentinel_cleanup)

/* Chain of cleanup actions established with make_cleanup,
to be executed if an error happens. */
+static struct cleanup *cleanup_chain = SENTINEL_CLEANUP;

-/* Cleaned up after a failed command. */
-static struct cleanup *cleanup_chain;
-
-/* Cleaned up when gdb exits. */
-static struct cleanup *final_cleanup_chain;
+/* Chain of cleanup actions established with make_final_cleanup,
+ to be executed when gdb exits. */
+static struct cleanup *final_cleanup_chain = SENTINEL_CLEANUP;

/* Main worker routine to create a cleanup.
PMY_CHAIN is a pointer to either cleanup_chain or final_cleanup_chain.
@@ -51,6 +89,7 @@ make_my_cleanup2 (struct cleanup **pmy_c
new->arg = arg;
*pmy_chain = new;

+ gdb_assert (old_chain != NULL);
return old_chain;
}

@@ -120,6 +159,15 @@ do_my_cleanups (struct cleanup **pmy_cha
}
}

+/* Return a value that can be passed to do_cleanups, do_final_cleanups to
+ indicate perform all cleanups. */
+
+struct cleanup *
+all_cleanups (void)
+{
+ return SENTINEL_CLEANUP;
+}
+
/* Discard cleanups and do the actions they describe
until we get back to the point OLD_CHAIN in the cleanup_chain. */

@@ -185,7 +233,7 @@ save_my_cleanups (struct cleanup **pmy_c
{
struct cleanup *old_chain = *pmy_chain;

- *pmy_chain = 0;
+ *pmy_chain = SENTINEL_CLEANUP;
return old_chain;
}

Index: cleanups.h
===================================================================
RCS file: /cvs/src/src/gdb/cleanups.h,v
retrieving revision 1.2
diff -u -p -r1.2 cleanups.h
--- cleanups.h 17 Apr 2012 21:24:47 -0000 1.2
+++ cleanups.h 19 Apr 2012 17:52:44 -0000
@@ -19,28 +19,8 @@
#ifndef CLEANUPS_H
#define CLEANUPS_H

-/* The cleanup list records things that have to be undone
- if an error happens (descriptors to be closed, memory to be freed, etc.)
- Each link in the chain records a function to call and an
- argument to give it.
-
- Use make_cleanup to add an element to the cleanup chain.
- Use do_cleanups to do all cleanup actions back to a given
- point in the chain. Use discard_cleanups to remove cleanups
- from the chain back to a given point, not doing them.
-
- If the argument is pointer to allocated memory, then you need
- to additionally set the 'free_arg' member to a function that will
- free that memory. This function will be called both when the cleanup
- is executed and when it's discarded. */
-
-struct cleanup
- {
- struct cleanup *next;
- void (*function) (void *);
- void (*free_arg) (void *);
- void *arg;
- };
+/* Outside of cleanups.c, this is an opaque type. */
+struct cleanup;

/* NOTE: cagney/2000-03-04: This typedef is strictly for the
make_cleanup function declarations below. Do not use this typedef
@@ -49,21 +29,25 @@ struct cleanup
Calling a f(char*) function with f(void*) is non-portable. */
typedef void (make_cleanup_ftype) (void *);

+/* Function type for the dtor in make_cleanup_dtor. */
+typedef void (make_cleanup_dtor_ftype) (void *);
+
/* WARNING: The result of the "make cleanup" routines is not the intuitive
choice of being a handle on the just-created cleanup. Instead it is an
opaque handle of the cleanup mechanism and represents all cleanups created
- from that point onwards. */
+ from that point onwards.
+ The result is guaranteed to be non-NULL though. */

extern struct cleanup *make_cleanup (make_cleanup_ftype *, void *);

extern struct cleanup *make_cleanup_dtor (make_cleanup_ftype *, void *,
- void (*dtor) (void *));
+ make_cleanup_dtor_ftype *);

extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);

/* A special value to pass to do_cleanups and do_final_cleanups
to tell them to do all cleanups. */
-#define ALL_CLEANUPS ((struct cleanup *)0)
+extern struct cleanup *all_cleanups (void);

extern void do_cleanups (struct cleanup *);
extern void do_final_cleanups (struct cleanup *);
Index: exceptions.c
===================================================================
RCS file: /cvs/src/src/gdb/exceptions.c,v
retrieving revision 1.50
diff -u -p -r1.50 exceptions.c
--- exceptions.c 4 Jan 2012 08:17:01 -0000 1.50
+++ exceptions.c 19 Apr 2012 17:52:44 -0000
@@ -224,7 +224,7 @@ throw_exception (struct gdb_exception ex
quit_flag = 0;
immediate_quit = 0;

- do_cleanups (ALL_CLEANUPS);
+ do_cleanups (all_cleanups ());

/* Jump to the containing catch_errors() call, communicating REASON
to that call via setjmp's return value. Note that REASON can't
Index: main.c
===================================================================
RCS file: /cvs/src/src/gdb/main.c,v
retrieving revision 1.108
diff -u -p -r1.108 main.c
--- main.c 17 Apr 2012 15:56:21 -0000 1.108
+++ main.c 19 Apr 2012 17:52:44 -0000
@@ -231,7 +231,7 @@ captured_command_loop (void *data)
are not that well behaved. do_cleanups should either be replaced
with a do_cleanups call (to cover the problem) or an assertion
check to detect bad FUNCs code. */
- do_cleanups (ALL_CLEANUPS);
+ do_cleanups (all_cleanups ());
/* If the command_loop returned, normally (rather than threw an
error) we try to quit. If the quit is aborted, catch_errors()
which called this catch the signal and restart the command
Index: top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.214
diff -u -p -r1.214 top.c
--- top.c 1 Mar 2012 19:30:20 -0000 1.214
+++ top.c 19 Apr 2012 17:52:44 -0000
@@ -1297,8 +1297,9 @@ quit_target (void *arg)
if (write_history_p && history_filename)
write_history (history_filename);

- do_final_cleanups (ALL_CLEANUPS); /* Do any final cleanups before
- exiting. */
+ /* Do any final cleanups before exiting. */
+ do_final_cleanups (all_cleanups ());
+
return 0;
}