On Mon, 19 Sep 2016 12:08:21 +0100 Eric Engestrom <[email protected]> wrote:
> On Mon, Sep 19, 2016 at 11:59:03AM +0100, Eric Engestrom wrote: > > On Fri, Sep 16, 2016 at 03:37:37PM -0700, Yong Bakos wrote: > > > From: Yong Bakos <[email protected]> > > > > > > Explicitly set the data member to NULL during wl_array_release, > > > preventing the > > > dangling pointer but, more importantly, making it testable. > > > > > > Signed-off-by: Yong Bakos <[email protected]> > > > --- > > > src/wayland-util.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/src/wayland-util.c b/src/wayland-util.c > > > index 639ccf8..2efb133 100644 > > > --- a/src/wayland-util.c > > > +++ b/src/wayland-util.c > > > @@ -102,6 +102,7 @@ WL_EXPORT void > > > wl_array_release(struct wl_array *array) > > > { > > > free(array->data); > > > + array->data = NULL; > > > > If we add > > array->size = 0; > > array->alloc = 0; > > > > we can then remove this comment from patch #1, right? > > \note Leaves the array in an invalid state. > > I somehow missed your cover-letter, but at least we agree :P > I guess you'll send that as an independent patch later on? Hi, yeah, this patch would essentially just insert a call to wl_array_init() at the end of wl_array_release(). I find it important to do this as a separate patch from patch 1 that documents the existing behaviour, so that the history on documentation also makes it clear the behaviour changes. Moreover, if you do this change in behaviour, you have to not delete the old documentation, but document that starting from libwayland version 1.x this function guarantees the wl_array is left in an initialized state. Now, if someone wanted to rely on the new behaviour, they would need to make sure that the runtime version of libwayland is new enough, otherwise they have a false assumption. Given that, I find it unlikely that anyone would actually want to re-use a wl_array after releasing and not call wl_array_init() again explicitly. Therefore, for the sake of exposing bugs and avoiding malfunction if a wl_array gets re-used unintendedly, I'd suggest to init it in a state where following uses of it would crash. I think setting the data pointer to an invalid non-NULL pointer could do. The test could rely on a bit of internal knowledge about what the invalid pointer value is, or even rely on a crash, actually. That reminds me that I wanted to consider changing the wl_list_remove() to set the prev/next to non-NULL invalid pointers than just NULL, since people can easily get the idea of checking if they are NULL while that is not supposed to work. I just wonder if that's too late. Thanks, pq > > > > The series is good anyway, so it is: > > Reviewed-by: Eric Engestrom <[email protected]> > _______________________________________________ > wayland-devel mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
pgpo7r8__o9G5.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
