From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14161 invoked by alias); 12 Jul 2008 22:53:21 -0000 Received: (qmail 14152 invoked by uid 22791); 12 Jul 2008 22:53:20 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 12 Jul 2008 22:52:45 +0000 Received: (qmail 19620 invoked from network); 12 Jul 2008 22:52:43 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 12 Jul 2008 22:52:43 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [continuation args 2/2] Make continuation args not leak Date: Sat, 12 Jul 2008 22:53:00 -0000 User-Agent: KMail/1.9.9 Cc: "Ulrich Weigand" , Daniel Jacobowitz References: <200807122023.m6CKNfDU019191@d12av02.megacenter.de.ibm.com> In-Reply-To: <200807122023.m6CKNfDU019191@d12av02.megacenter.de.ibm.com> MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_7WTeIAuu9SNns2Q" Message-Id: <200807122352.43828.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-07/txt/msg00243.txt.bz2 --Boundary-00=_7WTeIAuu9SNns2Q Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 1211 A Saturday 12 July 2008 21:23:41, Ulrich Weigand wrote: > Pedro Alves wrote: > > Rewrite continuations internals on top of cleanups and plug > > continuation arguments leaks. > > This breaks the build due to violation of the C aliasing rules: > > /home/uweigand/fsf/gdb-head/gdb/utils.c: In function 'add_continuation': > /home/uweigand/fsf/gdb-head/gdb/utils.c:479: warning: dereferencing > type-punned pointer will break strict-aliasing rules ... > > > + struct cleanup **as_cleanup_p = (struct cleanup **) &cmd_continuation; > > This may cause a "struct cleanup *" to alias with a "struct continuation > *", which is not allowed according to the C standard. > Sorry for that. Missed building at > -O0 to catch these things. I thought that since the type is not defined, this would not be a problem, as you can't dereference through cmd_continuation. > Why do we still have a "struct continuation" (as nowhere-defined type)? > Shouldn't this just use "struct cleanup" throughout? Being a cleanup is an implementation detail, I didn't want to make that fact public. This fixes the issue for me by making the inheritance explicit. Tested on x86_64-unknown-linux-gnu async mode. OK? -- Pedro Alves --Boundary-00=_7WTeIAuu9SNns2Q Content-Type: text/x-diff; charset="iso-8859-1"; name="continuations_alias.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="continuations_alias.diff" Content-length: 3897 2008-07-12 Pedro Alves * utils.c (struct continuation): Define as inheriting struct cleanup. (add_continuation, do_all_continuations) (discard_all_continuations, add_intermediate_continuation) (do_all_intermediate_continuations) (discard_all_intermediate_continuations): Adjust. --- gdb/utils.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) Index: src/gdb/utils.c =================================================================== --- src.orig/gdb/utils.c 2008-07-12 22:50:56.000000000 +0100 +++ src/gdb/utils.c 2008-07-12 23:25:40.000000000 +0100 @@ -470,19 +470,29 @@ null_cleanup (void *arg) { } +/* Continuations are implemented as cleanups internally. Inherit from + cleanups. */ +struct continuation +{ + struct cleanup base; +}; + /* Add a continuation to the continuation list, the global list - cmd_continuation. The new continuation will be added at the front.*/ + cmd_continuation. The new continuation will be added at the + front. */ void add_continuation (void (*continuation_hook) (void *), void *args, void (*continuation_free_args) (void *)) { - struct cleanup **as_cleanup_p = (struct cleanup **) &cmd_continuation; + struct cleanup *as_cleanup = &cmd_continuation->base; make_cleanup_ftype *continuation_hook_fn = continuation_hook; - make_my_cleanup2 (as_cleanup_p, + make_my_cleanup2 (&as_cleanup, continuation_hook_fn, args, continuation_free_args); + + cmd_continuation = (struct continuation *) as_cleanup; } /* Walk down the cmd_continuation list, and execute all the @@ -503,7 +513,7 @@ do_all_continuations (void) effect of invoking the continuations and the processing of the preexisting continuations will not be affected. */ - continuation_ptr = (struct cleanup *) cmd_continuation; + continuation_ptr = &cmd_continuation->base; cmd_continuation = NULL; /* Work now on the list we have set aside. */ @@ -515,8 +525,9 @@ do_all_continuations (void) void discard_all_continuations (void) { - struct cleanup **continuation_ptr = (struct cleanup **) &cmd_continuation; - discard_my_cleanups (continuation_ptr, NULL); + struct cleanup *continuation_ptr = &cmd_continuation->base; + discard_my_cleanups (&continuation_ptr, NULL); + cmd_continuation = NULL; } /* Add a continuation to the continuation list, the global list @@ -527,13 +538,15 @@ add_intermediate_continuation (void (*co (void *), void *args, void (*continuation_free_args) (void *)) { - struct cleanup **as_cleanup_p = (struct cleanup **) &intermediate_continuation; + struct cleanup *as_cleanup = &intermediate_continuation->base; make_cleanup_ftype *continuation_hook_fn = continuation_hook; - make_my_cleanup2 (as_cleanup_p, + make_my_cleanup2 (&as_cleanup, continuation_hook_fn, args, continuation_free_args); + + intermediate_continuation = (struct continuation *) as_cleanup; } /* Walk down the cmd_continuation list, and execute all the @@ -554,7 +567,7 @@ do_all_intermediate_continuations (void) effect of invoking the continuations and the processing of the preexisting continuations will not be affected. */ - continuation_ptr = (struct cleanup *) intermediate_continuation; + continuation_ptr = &intermediate_continuation->base; intermediate_continuation = NULL; /* Work now on the list we have set aside. */ @@ -566,9 +579,9 @@ do_all_intermediate_continuations (void) void discard_all_intermediate_continuations (void) { - struct cleanup **continuation_ptr - = (struct cleanup **) &intermediate_continuation; - discard_my_cleanups (continuation_ptr, NULL); + struct cleanup *continuation_ptr = &intermediate_continuation->base; + discard_my_cleanups (&continuation_ptr, NULL); + continuation_ptr = NULL; } --Boundary-00=_7WTeIAuu9SNns2Q--