Bug 74680 - [Plugins GTK/QT] Signed to Unsigned Conversion Error
Summary: [Plugins GTK/QT] Signed to Unsigned Conversion Error
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-15 20:10 PST by Kwangmin Bang
Modified: 2013-04-12 12:10 PDT (History)
1 user (show)

See Also:


Attachments
the patch file created by svn-create-patch (3.16 KB, patch)
2011-12-15 20:10 PST, Kwangmin Bang
no flags Details | Formatted Diff | Diff
recreate the patch with changelog (4.30 KB, patch)
2011-12-15 20:24 PST, Kwangmin Bang
eric: review-
Details | Formatted Diff | Diff
Signed to Unsigned Conversion Error (3.39 KB, patch)
2012-04-02 18:08 PDT, Kwangmin Bang
pnormand: review-
pnormand: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwangmin Bang 2011-12-15 20:10:31 PST
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.
Comment 1 Kwangmin Bang 2011-12-15 20:24:43 PST
Created attachment 119550 [details]
recreate the patch with changelog
Comment 2 Martin Robinson 2011-12-16 11:15:08 PST
Comment on attachment 119550 [details]
recreate the patch with changelog

In what situations can these values be negative?
Comment 3 Kwangmin Bang 2011-12-18 20:45:53 PST
it can be negative by something which can move viewport like scroller.
Comment 4 Eric Seidel (no email) 2011-12-21 12:23:55 PST
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?
Comment 5 Kwangmin Bang 2012-01-04 22:23:50 PST
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.
Comment 6 Kwangmin Bang 2012-04-02 18:08:15 PDT
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 ?
Comment 7 Gyuyoung Kim 2012-07-17 03:17:14 PDT
(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.
Comment 8 Philippe Normand 2013-04-12 12:10:38 PDT
Comment on attachment 135250 [details]
Signed to Unsigned Conversion Error

We need a test for this patch, please.