Bug 39010 - [Qt] QtField::name() returns a dangling pointer
Summary: [Qt] QtField::name() returns a dangling pointer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Anders Bakken
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-05-12 11:45 PDT by Anders Bakken
Modified: 2010-05-15 20:03 PDT (History)
2 users (show)

See Also:


Attachments
Patch that fixes the problem (3.00 KB, patch)
2010-05-12 11:48 PDT, Anders Bakken
kenneth: review-
Details | Formatted Diff | Diff
Patch that fixes the problem (3.07 KB, patch)
2010-05-13 08:30 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch for landing (3.23 KB, patch)
2010-05-15 17:56 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Bakken 2010-05-12 11:45:09 PDT
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.
Comment 1 Anders Bakken 2010-05-12 11:48:32 PDT
Created attachment 55879 [details]
Patch that fixes the problem
Comment 2 Jesus Sanchez-Palencia 2010-05-13 07:07:31 PDT
(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!
Comment 3 Kenneth Rohde Christiansen 2010-05-13 07:55:58 PDT
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 @@
> +2010-05-12  Anders Bakken  <agbakken@gmail.com>
> +
> +        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.
Comment 4 Anders Bakken 2010-05-13 08:30:19 PDT
Created attachment 55985 [details]
Patch that fixes the problem
Comment 5 Kenneth Rohde Christiansen 2010-05-13 09:33:19 PDT
Don't forget to add the r? and cq? flags
Comment 6 WebKit Commit Bot 2010-05-15 08:51:06 PDT
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 eseidel@chromium.org...
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).
Comment 7 Kenneth Rohde Christiansen 2010-05-15 12:31:51 PDT
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.
Comment 8 Adam Barth 2010-05-15 17:56:40 PDT
Created attachment 56167 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2010-05-15 20:03:50 PDT
Comment on attachment 56167 [details]
Patch for landing

Clearing flags on attachment: 56167

Committed r59564: <http://trac.webkit.org/changeset/59564>
Comment 10 WebKit Commit Bot 2010-05-15 20:03:56 PDT
All reviewed patches have been landed.  Closing bug.