[GTK][WPE] Minor code cleanup in BubblewrapLauncher
Created attachment 379023 [details] Patch
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
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.
(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.
Created attachment 379376 [details] Patch
Comment on attachment 379376 [details] Patch LGTM, it seems it needs to be rebased.
Created attachment 379473 [details] Patch
Comment on attachment 379473 [details] Patch Clearing flags on attachment: 379473 Committed r250320: <https://trac.webkit.org/changeset/250320>
All reviewed patches have been landed. Closing bug.