[PATCH wayland 2/4] wl_array: Set data to null after free

[Pekka Paalanen]

On Mon, 19 Sep 2016 12:08:21 +0100
Eric Engestrom <eric.engestrom at imgtec.com> 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 <ybakos at humanoriented.com>
> > > 
> > > 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 <ybakos at humanoriented.com>
> > > ---
> > >  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 <eric.engestrom at imgtec.com>  
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160920/8704f521/attachment.sig>