On 7 March 2018 at 20:10, Alan Coopersmith <[email protected]> wrote: > On 03/ 7/18 05:36 AM, Emil Velikov wrote: >> Hi Alan, >> >> On 6 March 2018 at 21:47, Alan Coopersmith <[email protected]> >> wrote: >>> Reported by gcc 7.3: >>> >>> GetImage.c:110:25: warning: potential null pointer dereference >>> [-Wnull-dereference] >>> if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 || >>> ~~~~~^~~~~~~~ >>> >>> Introduced by 8ea762f94f4c942d898fdeb590a1630c83235c17 in Xlib 1.6.4 >>> >>> Signed-off-by: Alan Coopersmith <[email protected]> >>> --- >>> src/GetImage.c | 16 +++++++++------- >>> 1 file changed, 9 insertions(+), 7 deletions(-) >>> >>> diff --git a/src/GetImage.c b/src/GetImage.c >>> index ff32d589..44a576a1 100644 >>> --- a/src/GetImage.c >>> +++ b/src/GetImage.c >>> @@ -105,14 +105,16 @@ XImage *XGetImage ( >>> planes = 1; >>> } >>> >>> - if (!image) >>> + if (!image) { >>> Xfree(data); >>> - if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 || >>> - INT_MAX / image->height <= image->bytes_per_line || >>> - INT_MAX / planes <= image->height * image->bytes_per_line || >>> - nbytes < planes * image->height * image->bytes_per_line) { >>> - XDestroyImage(image); >>> - image = NULL; >>> + } else { >>> + if (planes < 1 || image->height < 1 || image->bytes_per_line < >>> 1 || >>> + INT_MAX / image->height <= image->bytes_per_line || >>> + INT_MAX / planes <= image->height * image->bytes_per_line >>> || >>> + nbytes < planes * image->height * image->bytes_per_line) { >>> + XDestroyImage(image); >>> + image = NULL; >>> + } >> >> Instead of reshuffling, one could easily add the missing unlock/sync >> in the error path. >> Or even a goto unlock? > > Sorry - not sure I'm following - are you suggesting something like this? > > --- a/src/GetImage.c > +++ b/src/GetImage.c > @@ -104,17 +104,20 @@ XImage *XGetImage ( > _XGetScanlinePad(dpy, (int) rep.depth), 0); > planes = 1; > } > > - if (!image) > + if (!image) { > Xfree(data); > + goto unlock; > + } > if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 || > INT_MAX / image->height <= image->bytes_per_line || > INT_MAX / planes <= image->height * image->bytes_per_line || > nbytes < planes * image->height * image->bytes_per_line) { > XDestroyImage(image); > image = NULL; > } > + unlock: > UnlockDisplay(dpy); > SyncHandle(); > return (image); > } > Yes, precisely.
> > That should be effectively equivalent, just less change in indentation for > the lines in between. > You're correct - it's functionally identical although it seems cleaner. Regardless of which version you opt for Reviewed-by: Emil Velikov <[email protected]> -Emil _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
