[Mac] Remove potential for malloc in CrMallocErrorBreak().

[Mac] Remove potential for malloc in CrMallocErrorBreak().

Calling into malloc library from the malloc library error handler is
probably a bad idea.

BUG=160555
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167722

[Robert Sesek, 2012-11-12 22:17:13]

LGTM

https://codereview.chromium.org/11363208/diff/1/base/process_util_mac.mm
File base/process_util_mac.mm (right):

https://codereview.chromium.org/11363208/diff/1/base/process_util_mac.mm#newc...
base/process_util_mac.mm:610: size_t bl = strlen(buf);
naming: |bl| -> |bufLen| ?

[Scott Hess - ex-Googler, 2012-11-12 22:31:52]

+mark for OWNERS review.

https://codereview.chromium.org/11363208/diff/1/base/process_util_mac.mm
File base/process_util_mac.mm (right):

https://codereview.chromium.org/11363208/diff/1/base/process_util_mac.mm#newc...
base/process_util_mac.mm:610: size_t bl = strlen(buf);
On 2012/11/12 22:17:13, rsesek wrote:
> naming: |bl| -> |bufLen| ?

Done.

[Mark Mentovai, 2012-11-12 22:45:10]

https://codereview.chromium.org/11363208/diff/3002/base/process_util_mac.mm
File base/process_util_mac.mm (right):

https://codereview.chromium.org/11363208/diff/3002/base/process_util_mac.mm#n...
base/process_util_mac.mm:610: size_t bufLen = strlen(buf);
Nit: buf_len.

https://codereview.chromium.org/11363208/diff/3002/base/process_util_mac.mm#n...
base/process_util_mac.mm:611: safe_strerror_r(errno, buf + bufLen, sizeof(buf) -
bufLen);
[safe_]strerror[_r] might wind up entering the allocator to access localized
messages. You can use the deprecated sys_errlist variable (along with sys_nerr),
but I’d just as soon drop the friendly string in this case and just use the
numeric value of errno.

For that matter, it’s a real pain to prove that the printf family can’t enter
the allocator for a given use either. snprintf with only integral and string
types SHOULD be safe.

[Scott Hess - ex-Googler, 2012-11-12 23:06:43]

Humans can deal with the leading 0's.  Or the errno can just be omitted, it's
only there because of unexpected edge cases cases like the EBADF just above.

https://codereview.chromium.org/11363208/diff/3002/base/process_util_mac.mm
File base/process_util_mac.mm (right):

https://codereview.chromium.org/11363208/diff/3002/base/process_util_mac.mm#n...
base/process_util_mac.mm:611: safe_strerror_r(errno, buf + bufLen, sizeof(buf) -
bufLen);
On 2012/11/12 22:45:10, Mark Mentovai wrote:
> [safe_]strerror[_r] might wind up entering the allocator to access localized
> messages. You can use the deprecated sys_errlist variable (along with
sys_nerr),
> but I’d just as soon drop the friendly string in this case and just use the
> numeric value of errno.

Seems reasonable.

> For that matter, it’s a real pain to prove that the printf family can’t enter
> the allocator for a given use either. snprintf with only integral and string
> types SHOULD be safe.

I actually recently had opportunity to audit that code, and AFAICT it will only
dynamically allocate in certain cases of floating-point output, and vector
output.  But, even so ... there's always MATH.

[Mark Mentovai, 2012-11-12 23:19:32]

LGTM

https://codereview.chromium.org/11363208/diff/4/base/process_util_mac.mm
File base/process_util_mac.mm (right):

https://codereview.chromium.org/11363208/diff/4/base/process_util_mac.mm#newc...
base/process_util_mac.mm:609: "errno==";
One = too many?

https://codereview.chromium.org/11363208/diff/4/base/process_util_mac.mm#newc...
base/process_util_mac.mm:610: char errnobuf[4] = {
No [4], just [].

https://codereview.chromium.org/11363208/diff/4/base/process_util_mac.mm#newc...
base/process_util_mac.mm:615: };
Ha!

Do you want to COMPILE_ASSERT that ELAST <= 999?

https://codereview.chromium.org/11363208/diff/4/base/process_util_mac.mm#newc...
base/process_util_mac.mm:616: strlcat(buf, errnobuf, sizeof(buf));
I can think of a way to avoid the copy, but that’s just for fun at that point,
there’s nothing wrong with the strlcat.

[Scott Hess - ex-Googler, 2012-11-12 23:39:53]

This and cases like https://codereview.chromium.org/11362048/ kinda make me want
to find an appropriately-licensed variant of simple_snprintf() like the OSX
malloc library uses when all hell has broken loose.

https://codereview.chromium.org/11363208/diff/4/base/process_util_mac.mm
File base/process_util_mac.mm (right):

https://codereview.chromium.org/11363208/diff/4/base/process_util_mac.mm#newc...
base/process_util_mac.mm:609: "errno==";
On 2012/11/12 23:19:32, Mark Mentovai wrote:
> One = too many?

Done.  I was thinking C-style is-equal-to, but a byte is a byte.

https://codereview.chromium.org/11363208/diff/4/base/process_util_mac.mm#newc...
base/process_util_mac.mm:610: char errnobuf[4] = {
On 2012/11/12 23:19:32, Mark Mentovai wrote:
> No [4], just [].

Done.

https://codereview.chromium.org/11363208/diff/4/base/process_util_mac.mm#newc...
base/process_util_mac.mm:615: };
On 2012/11/12 23:19:32, Mark Mentovai wrote:
> Ha!
> 
> Do you want to COMPILE_ASSERT that ELAST <= 999?

Honestly, I kind of questioned even the third digit.  But, sure.

https://codereview.chromium.org/11363208/diff/4/base/process_util_mac.mm#newc...
base/process_util_mac.mm:616: strlcat(buf, errnobuf, sizeof(buf));
On 2012/11/12 23:19:32, Mark Mentovai wrote:
> I can think of a way to avoid the copy, but that’s just for fun at that point,
> there’s nothing wrong with the strlcat.

There are dozens of ways to skin this cat, and they're all guaranteed to be
gross.

[commit-bot: I haz the power, 2012-11-14 17:38:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/11363208/10001

[commit-bot: I haz the power, 2012-11-14 20:11:14]

Change committed as 167722