| Message ID | 20231213065201.886391-1-frolov@swemel.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ui: fix DIV_BY_ZERO in tightvnc | expand |
Hi On Wed, Dec 13, 2023 at 11:08 AM Dmitry Frolov <frolov@swemel.ru> wrote: > > Division by zero may occur in rare constellation of conditions if: > 1. not TrueColor mode on the client side > tight_detect_smooth_image16() and tight_detect_smooth_image32(), > defined by macro DEFINE_DETECT_FUNCTION()2, are affected. > 2. if all pixels on the screen are equal, then pixels == stats[0] > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Dmitry Frolov <frolov@swemel.ru> What about the tight_detect_smooth_image24() division? errors /= (pixels * 3 - stats[0]); It should probably have a similar safety check. The code is originally from libvncserver, but they completely changed their implementation in: https://github.com/LibVNC/libvncserver/commit/7124b5fbcf0df8db4d3f73023d77af6ea56409e7 otherwise, Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > ui/vnc-enc-tight.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c > index 41f559eb83..f1249ab136 100644 > --- a/ui/vnc-enc-tight.c > +++ b/ui/vnc-enc-tight.c > @@ -284,6 +284,9 @@ tight_detect_smooth_image24(VncState *vs, int w, int h) > for (; c < 256; c++) { \ > errors += stats[c] * (c * c); \ > } \ > + if (pixels == stats[0]) { \ > + return 0; \ > + } \ > errors /= (pixels - stats[0]); \ > \ > return errors; \ > -- > 2.34.1 >
On 13.12.2023 10:31, Marc-André Lureau wrote: > Hi > > On Wed, Dec 13, 2023 at 11:08 AM Dmitry Frolov <frolov@swemel.ru> wrote: >> Division by zero may occur in rare constellation of conditions if: >> 1. not TrueColor mode on the client side >> tight_detect_smooth_image16() and tight_detect_smooth_image32(), >> defined by macro DEFINE_DETECT_FUNCTION()2, are affected. >> 2. if all pixels on the screen are equal, then pixels == stats[0] >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Signed-off-by: Dmitry Frolov <frolov@swemel.ru> > What about the tight_detect_smooth_image24() division? > errors /= (pixels * 3 - stats[0]); Here everything is OK, because there is a check some lines above: if (stats[0] * 33 / pixels >= 95) { return 0; } thus, stats[0] < pixels*95/33, 95/33 < 3. > > It should probably have a similar safety check. > > The code is originally from libvncserver, but they completely changed > their implementation in: > https://github.com/LibVNC/libvncserver/commit/7124b5fbcf0df8db4d3f73023d77af6ea56409e7 > otherwise, > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> --- >> ui/vnc-enc-tight.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c >> index 41f559eb83..f1249ab136 100644 >> --- a/ui/vnc-enc-tight.c >> +++ b/ui/vnc-enc-tight.c >> @@ -284,6 +284,9 @@ tight_detect_smooth_image24(VncState *vs, int w, int h) >> for (; c < 256; c++) { \ >> errors += stats[c] * (c * c); \ >> } \ >> + if (pixels == stats[0]) { \ >> + return 0; \ >> + } \ >> errors /= (pixels - stats[0]); \ >> \ >> return errors; \ >> -- >> 2.34.1 >>
Hi On Wed, Dec 13, 2023 at 11:39 AM Дмитрий Фролов <frolov@swemel.ru> wrote: > > > > On 13.12.2023 10:31, Marc-André Lureau wrote: > > Hi > > > > On Wed, Dec 13, 2023 at 11:08 AM Dmitry Frolov <frolov@swemel.ru> wrote: > >> Division by zero may occur in rare constellation of conditions if: > >> 1. not TrueColor mode on the client side > >> tight_detect_smooth_image16() and tight_detect_smooth_image32(), > >> defined by macro DEFINE_DETECT_FUNCTION()2, are affected. > >> 2. if all pixels on the screen are equal, then pixels == stats[0] > >> > >> Found by Linux Verification Center (linuxtesting.org) with SVACE. > >> > >> Signed-off-by: Dmitry Frolov <frolov@swemel.ru> > > What about the tight_detect_smooth_image24() division? > > errors /= (pixels * 3 - stats[0]); > Here everything is OK, because there is a check some lines above: > if (stats[0] * 33 / pixels >= 95) { > return 0; > } > thus, stats[0] < pixels*95/33, > 95/33 < 3. > > /me shivers.. looks legit, yet so easy to get wrong :) > > It should probably have a similar safety check. > > > > The code is originally from libvncserver, but they completely changed > > their implementation in: > > https://github.com/LibVNC/libvncserver/commit/7124b5fbcf0df8db4d3f73023d77af6ea56409e7 > > otherwise, > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > >> --- > >> ui/vnc-enc-tight.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c > >> index 41f559eb83..f1249ab136 100644 > >> --- a/ui/vnc-enc-tight.c > >> +++ b/ui/vnc-enc-tight.c > >> @@ -284,6 +284,9 @@ tight_detect_smooth_image24(VncState *vs, int w, int h) > >> for (; c < 256; c++) { \ > >> errors += stats[c] * (c * c); \ > >> } \ > >> + if (pixels == stats[0]) { \ > >> + return 0; \ > >> + } \ > >> errors /= (pixels - stats[0]); \ > >> \ > >> return errors; \ > >> -- > >> 2.34.1 > >> >
diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c index 41f559eb83..f1249ab136 100644 --- a/ui/vnc-enc-tight.c +++ b/ui/vnc-enc-tight.c @@ -284,6 +284,9 @@ tight_detect_smooth_image24(VncState *vs, int w, int h) for (; c < 256; c++) { \ errors += stats[c] * (c * c); \ } \ + if (pixels == stats[0]) { \ + return 0; \ + } \ errors /= (pixels - stats[0]); \ \ return errors; \
Division by zero may occur in rare constellation of conditions if: 1. not TrueColor mode on the client side tight_detect_smooth_image16() and tight_detect_smooth_image32(), defined by macro DEFINE_DETECT_FUNCTION()2, are affected. 2. if all pixels on the screen are equal, then pixels == stats[0] Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Dmitry Frolov <frolov@swemel.ru> --- ui/vnc-enc-tight.c | 3 +++ 1 file changed, 3 insertions(+)