[Plugins GTK/QT] Signed to Unsigned Conversion Error

• Component: Plug-ins
• Status: RESOLVED WONTFIX

[Kwangmin Bang]

Created attachment 119548 [details]
the patch file created by svn-create-patch

If signed integer variables having negative number is allocated to unsigned short variables,
unsigned short variables will have an unexpected value.

So, if signed integer variable has negative number,fill unsigned short variables to zero before type casting.
If not, clipRect(unsigned short) have an unexpected value cause of the first bit for negative.

[Kwangmin Bang]

Created attachment 119550 [details]
recreate the patch with changelog

[Martin Robinson]

Comment on attachment 119550 [details]
recreate the patch with changelog

In what situations can these values be negative?

[Kwangmin Bang]

it can be negative by something which can move viewport like scroller.

[Eric Seidel (no email)]

Comment on attachment 119550 [details]
recreate the patch with changelog

View in context: https://bugs.webkit.org/attachment.cgi?id=119550&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This will cause the commit-queue to fail.  You shoul deithe rlist the tests affected by this change, or explain why this change is impossible/impractical to test.

We have a TestPlugin in DRT which can be helpful if you need to make a test which involves NPAPI code.  Otherwise most tests use javascript to poke and prod at things, or dump the painted pixels and compare them against a reference.

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:527
> +        // Clipping rectangle of the plug-in; the origin is the top left corner of the drawable or window.
> +        // Signed to Unsigned Conversion Error; if signed integer variable has negative number,
> +        // fill unsigned short variables to zero before type casting.
> +        // If not, clipRect have an unexpected value cause of the first bit for negative.
> +        m_npWindow.clipRect.left = max(0, m_npWindow.x + m_clipRect.x());
> +        m_npWindow.clipRect.top = max(0, m_npWindow.y + m_clipRect.y());
> +        m_npWindow.clipRect.right = max(0, m_npWindow.x + m_clipRect.x() + m_clipRect.width());
> +        m_npWindow.clipRect.bottom = max(0, m_npWindow.y + m_clipRect.y() + m_clipRect.height());

Seems we should be sharing this code on the root PluginView implementation with a private helper method, no?

[Kwangmin Bang]

i'll upload the patch again after test.

And I think this patch should be shared to gtk/mac/qt platform having setNPWindowIfNeeded().
it is not need to implement to root Plugiview because some platform does not use setNPWindowIfNeeded() including this problem.

[Kwangmin Bang]

Created attachment 135250 [details]
Signed to Unsigned Conversion Error

rebase the patch.

When tested webkit for desktop, it's not need this patch.
My testing environment is using floating viewport for mobile browser( view is bigger than device screen).
For getting m_windowRect, I'm using contentsToScreen instead of contentsToWindow.
so, m_windowRect.x or y can be the negative.

However, I think these codes are a basic problem about conversion. ( unsigned = signed )
this patch will prevent "Signed to Unsigned Conversion Error" and remove the warning in build time.

How do you think about my opinion ?

[Gyuyoung Kim]

(In reply to comment #6)
> Created an attachment (id=135250) [details]
> Signed to Unsigned Conversion Error
> 
> rebase the patch.
> 
> When tested webkit for desktop, it's not need this patch.
> My testing environment is using floating viewport for mobile browser( view is bigger than device screen).
> For getting m_windowRect, I'm using contentsToScreen instead of contentsToWindow.
> so, m_windowRect.x or y can be the negative.
> 
> However, I think these codes are a basic problem about conversion. ( unsigned = signed )
> this patch will prevent "Signed to Unsigned Conversion Error" and remove the warning in build time.
> 
> How do you think about my opinion ?

As Eric said on comment #4, I think reviewers can accept this patch when you submit this patch with test cases. And, if you have test cases for this patch, I also think this patch can be adjusted to root file.

[Philippe Normand]

Comment on attachment 135250 [details]
Signed to Unsigned Conversion Error

We need a test for this patch, please.

[Ahmad Saleem]

NPAPI support is removed from Safari 14 onward and it is not supported in Webkit Builds like WebkitGTK as well. I think this can be marked as "RESOLVED WONTFIX". Thanks!

[Ryosuke Niwa]

Yup, won't fix.