Bug 201906 – [GTK][WPE] Minor code cleanup in BubblewrapLauncher

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Patrick Griffis]

[GTK][WPE] Minor code cleanup in BubblewrapLauncher

[Patrick Griffis]

Created attachment 379023 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 379023 [details]
Patch

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

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:133
> -        m_proxyPath = makeProxyPath(appRunDir.get()).get();
> +        m_proxyPath = makeProxyPath(appRunDir.get()).release();

What does this clarify? the string is always going to be copied by CString constructor, there's no ownership transfer. I think this would be less confusing if makeProxyPath returned a CString instead of a GUniquePtr<char>.

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:136
> -        m_path = dbusPath.get();
> +        m_path = dbusPath.release();

Same here, dbusAddressToPath should return a CString

[Michael Catanzaro]

BTW I thought these were UAF vulnerabilities because I didn't realize the CString constructor does a memcpy... so I thought .release() would be clearer and bullied Patrick into changing this. Don't blame him. ;)

I agree that using CString throughout will be better.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #3)
> BTW I thought these were UAF vulnerabilities because I didn't realize the
> CString constructor does a memcpy... so I thought .release() would be
> clearer and bullied Patrick into changing this. Don't blame him. ;)

:-) I don't blame anybody in any case, I was just confused by the clarification :-D

> I agree that using CString throughout will be better.

[Patrick Griffis]

Created attachment 379376 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 379376 [details]
Patch

LGTM, it seems it needs to be rebased.

[Patrick Griffis]

Created attachment 379473 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 379473 [details]
Patch

Clearing flags on attachment: 379473

Committed r250320: <https://trac.webkit.org/changeset/250320>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.