[Mac] Make the local_discovery client more resilient to invalid UTF-8.

[Mac] Make the local_discovery client more resilient to invalid UTF-8.

Both service names and TXT records could contain invalid code unit sequences
that could later lead to crashes. This change also fixes several NSObject leaks.

BUG=586628
Committed: https://crrev.com/4de054ddd802a66e912b5d1e29666d58413148ff
Cr-Commit-Position: refs/heads/master@{#404781}

[Robert Sesek, 2016-07-08 17:12:02]

Patchset #1 (id:1) has been deleted

[Robert Sesek, 2016-07-08 17:53:34]

Patchset #1 (id:20001) has been deleted

[Robert Sesek, 2016-07-08 17:54:10]

rsesek@chromium.org changed reviewers:
+ vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2016-07-11 20:30:25]

https://codereview.chromium.org/2132723003/diff/40001/chrome/browser/local_di...
File chrome/browser/local_discovery/service_discovery_client_mac.mm (right):

https://codereview.chromium.org/2132723003/diff/40001/chrome/browser/local_di...
chrome/browser/local_discovery/service_discovery_client_mac.mm:75:
type->reset(base::SysUTF8ToNSString(service.substr(0, last_period) + "."),
Maybe safer to do all string transformations after conversion from UTF8?

[Robert Sesek, 2016-07-11 20:35:18]

https://codereview.chromium.org/2132723003/diff/40001/chrome/browser/local_di...
File chrome/browser/local_discovery/service_discovery_client_mac.mm (right):

https://codereview.chromium.org/2132723003/diff/40001/chrome/browser/local_di...
chrome/browser/local_discovery/service_discovery_client_mac.mm:75:
type->reset(base::SysUTF8ToNSString(service.substr(0, last_period) + "."),
On 2016/07/11 20:30:25, Vitaly Buka wrote:
> Maybe safer to do all string transformations after conversion from UTF8?

I don't think it's any less safe to do it this way. The return value conveys
whether the conversions succeeded.

[Vitaly Buka (NO REVIEWS), 2016-07-11 20:48:24]

lgtm

https://codereview.chromium.org/2132723003/diff/40001/chrome/browser/local_di...
File chrome/browser/local_discovery/service_discovery_client_mac.mm (right):

https://codereview.chromium.org/2132723003/diff/40001/chrome/browser/local_di...
chrome/browser/local_discovery/service_discovery_client_mac.mm:75:
type->reset(base::SysUTF8ToNSString(service.substr(0, last_period) + "."),
On 2016/07/11 20:35:18, Robert Sesek wrote:
> On 2016/07/11 20:30:25, Vitaly Buka wrote:
> > Maybe safer to do all string transformations after conversion from UTF8?
> 
> I don't think it's any less safe to do it this way. The return value conveys
> whether the conversions succeeded.

'safer' -> 'cleaner'

My concerns is that find_last_of for ascii char and utf8 string is not correct
in general. Although it should work for '.'

[Vitaly Buka (NO REVIEWS), 2016-07-11 20:50:07]

On 2016/07/11 20:48:24, Vitaly Buka wrote:
> lgtm
> 
>
https://codereview.chromium.org/2132723003/diff/40001/chrome/browser/local_di...
> File chrome/browser/local_discovery/service_discovery_client_mac.mm (right):
> 
>
https://codereview.chromium.org/2132723003/diff/40001/chrome/browser/local_di...
> chrome/browser/local_discovery/service_discovery_client_mac.mm:75:
> type->reset(base::SysUTF8ToNSString(service.substr(0, last_period) + "."),
> On 2016/07/11 20:35:18, Robert Sesek wrote:
> > On 2016/07/11 20:30:25, Vitaly Buka wrote:
> > > Maybe safer to do all string transformations after conversion from UTF8?
> > 
> > I don't think it's any less safe to do it this way. The return value conveys
> > whether the conversions succeeded.
> 
> 'safer' -> 'cleaner'
> 
> My concerns is that find_last_of for ascii char and utf8 string is not correct
> in general. Although it should work for '.'

BTW, i have no access to the bug, can you CC me there?

[Robert Sesek, 2016-07-11 20:56:08]

> BTW, i have no access to the bug, can you CC me there?

Done.

https://codereview.chromium.org/2132723003/diff/40001/chrome/browser/local_di...
File chrome/browser/local_discovery/service_discovery_client_mac.mm (right):

https://codereview.chromium.org/2132723003/diff/40001/chrome/browser/local_di...
chrome/browser/local_discovery/service_discovery_client_mac.mm:75:
type->reset(base::SysUTF8ToNSString(service.substr(0, last_period) + "."),
On 2016/07/11 20:48:24, Vitaly Buka wrote:
> On 2016/07/11 20:35:18, Robert Sesek wrote:
> > On 2016/07/11 20:30:25, Vitaly Buka wrote:
> > > Maybe safer to do all string transformations after conversion from UTF8?
> > 
> > I don't think it's any less safe to do it this way. The return value conveys
> > whether the conversions succeeded.
> 
> 'safer' -> 'cleaner'
> 
> My concerns is that find_last_of for ascii char and utf8 string is not correct
> in general. Although it should work for '.'

find_last_of isn't going to be searching code points though, it'll just go by
bytes. std::string generally doesn't  care about the validity of the bytes
within it.

[Vitaly Buka (NO REVIEWS), 2016-07-11 21:26:30]

lgtm

[Robert Sesek, 2016-07-12 01:06:30]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2016-07-12 01:07:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 04:12:53]

Committed patchset #1 (id:40001)

[commit-bot: I haz the power, 2016-07-12 04:13:00]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-12 04:14:53]

Description was changed from

==========
[Mac] Make the local_discovery client more resilient to invalid UTF-8.

Both service names and TXT records could contain invalid code unit sequences
that could later lead to crashes. This change also fixes several NSObject leaks.

BUG=586628
==========

to

==========
[Mac] Make the local_discovery client more resilient to invalid UTF-8.

Both service names and TXT records could contain invalid code unit sequences
that could later lead to crashes. This change also fixes several NSObject leaks.

BUG=586628

Committed: https://crrev.com/4de054ddd802a66e912b5d1e29666d58413148ff
Cr-Commit-Position: refs/heads/master@{#404781}
==========

[commit-bot: I haz the power, 2016-07-12 04:14:54]

Patchset 1 (id:??) landed as
https://crrev.com/4de054ddd802a66e912b5d1e29666d58413148ff
Cr-Commit-Position: refs/heads/master@{#404781}