On one hand it may be dangerous for the scenario that you've described, but on the other hand why are you (or anyone) needing to call internal, non-exported libwayland functions?
?? On Thu, Oct 19, 2017 at 3:11 AM Boram Park <[email protected]> wrote: > > > On 2017년 09월 28일 00:13, Derek Foreman wrote: > > On 2017-09-26 10:46 AM, Sergi Granell wrote: > >> On Thu, 2017-09-14 at 09:21 +0900, Boram Park wrote: > >>> Before putting data into a buffer, we have to make sure that the data > >>> size is > >>> smaller than not only the buffer's full size but also the buffer's > >>> empty > >>> size. > >>> > >>> https://bugs.freedesktop.org/show_bug.cgi?id=102690 > >>> > >>> Signed-off-by: Boram Park <[email protected]> > >>> Acked-by: Pekka Paalanen <[email protected]> > >>> --- > >>> src/connection.c | 9 ++++++--- > >>> 1 file changed, 6 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/src/connection.c b/src/connection.c > >>> index 5c3d187..53b1621 100644 > >>> --- a/src/connection.c > >>> +++ b/src/connection.c > >>> @@ -63,14 +63,17 @@ struct wl_connection { > >>> int want_flush; > >>> }; > >>> +static uint32_t wl_buffer_size(struct wl_buffer *b); > >>> + > >> > >> I think it would be a better idea to move the wl_buffer_size definition > >> at the top to avoid this forward declaration. > >> > >>> static int > >>> wl_buffer_put(struct wl_buffer *b, const void *data, size_t count) > >>> { > >>> - uint32_t head, size; > >>> + uint32_t head, size, empty; > >>> - if (count > sizeof(b->data)) { > >>> + empty = sizeof(b->data) - wl_buffer_size(b); > >>> + if (count > empty) { > >>> wl_log("Data too big for buffer (%d > %d).\n", > >>> - count, sizeof(b->data)); > >>> + count, empty); > >>> errno = E2BIG; > >>> return -1; > >>> } > > > > I'm not sure I like this. I've looked and all callers should already > > have this check - are you actually getting here with this condition > > somehow? > looks it will never happen because all callers already check the data > size before putting. > > Also, the patch changes the meaning of E2BIG from the caller's > > perspective (if we can even get here), doesn't it? Previously E2BIG > > meant the packet could never fit, now it would mean that the packet > > can't fit now. > > > > I think maybe just a comment mentioning that all the callers must > > ensure the data will fit could be enough? > However, it looks really dangerous for someone who don't know above rule > that caller should check the buffer empty size before calling > wl_buffer_put. > comment might be helpful to understand the intention of developer who > implemented this code. > But the sanity check is much better to ensure safety, I think. > > > > > I could even see an assert(), since this is a conditional that should > > never fire. > > > > But I really don't like changing the meaning of the error code. > > > > Thanks, > > Derek > > > >> Other than that, > >> > >> Reviewed-by: Sergi Granell <xerpi.g.12 at gmail.com> > >> _______________________________________________ > >> wayland-devel mailing list > >> [email protected] > >> https://lists.freedesktop.org/mailman/listinfo/wayland-devel > >> > > > > _______________________________________________ > > wayland-devel mailing list > > [email protected] > > https://lists.freedesktop.org/mailman/listinfo/wayland-devel > > _______________________________________________ > wayland-devel mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/wayland-devel >
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
