Bug 39010 – [Qt] QtField::name() returns a dangling pointer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Anders Bakken]

QObject::objectName() returns a QString and the QtField::name() creates a temporary QByteArray object and returns constData() from it.
This is not safe.
The attached fix changes the signature to be a QByteArray instead of const char* and makes it non-virtual (since it never should have been). It's not a Bindings::Field function but rather an implementation detail in QtField and it's never reimplemented.

The patch also includes a minor optimization to use const & in a foreach loop which is faster.

[Anders Bakken]

Created attachment 55879 [details]
Patch that fixes the problem

[Jesus Sanchez-Palencia]

(In reply to comment #1)
> Created an attachment (id=55879) [details]
> Patch that fixes the problem

Did you forget to mark it for review?! :)

Quick tips for your next bug reports:

- please add the 'Qt' Keyword;
- if the bug is Qt specific add a '[Qt]' to the Summary;


Thanks!

[Kenneth Rohde Christiansen]

Comment on attachment 55879 [details]
Patch that fixes the problem

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 59246)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> [phone]  Anders Bakken  <[e-mail]>
> +
> +        QObject::objectName() returns a QString and QtField::name()
> +	creates a temporary QByteArray object and returns constData() from it.
> +	This is not safe.
> +
> +	This patch changes the signature of the function to return a
> +	QByteArray instead of const char *.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=39010
> +
> +        * bridge/qt/qt_instance.cpp:
> +        (JSC::Bindings::QtInstance::getPropertyNames):
> +        (JSC::Bindings::QtField::name):
> +        * bridge/qt/qt_runtime.h:
> +

Please fix the indentation
WebCore/bridge/qt/qt_instance.cpp:234
 +          for (i=0; i < methodCount; i++) {
i = 0, please fix the spacing

Apart from this is look fine.

[Anders Bakken]

Created attachment 55985 [details]
Patch that fixes the problem

[Kenneth Rohde Christiansen]

Don't forget to add the r? and cq? flags

[WebKit Commit Bot]

Comment on attachment 55985 [details]
Patch that fixes the problem

Rejecting patch 55985 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 55985, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=55985&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=39010&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 55985 from bug 39010.
ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Kenneth Rohde Christiansen]

Anders, you cannot remove the "Reviewed by NOBODY (OOPS!!)". If you do that we cannot land using the commit-queue. It has to be exactly like the prepare-ChangeLog scripts writes it.

Now you can reupload the patch with this fixed and reset r? and cq? 

-- or --

You can upload the patch with "Reviewed by Kenneth Rohde Christiansen" and just set cq?

The latter is preferred as anyone with committer-rights can change the cq? to cq+

Another option would be to get someone to land it manually using webkit-patch or similar.

[Adam Barth]

Created attachment 56167 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 56167 [details]
Patch for landing

Clearing flags on attachment: 56167

Committed r59564: <http://trac.webkit.org/changeset/59564>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.