[macOS] The function getpwnam can sometimes fail

[Per Arne Vollan]

The function getpwnam can sometimes fail in the WebContent and GPU process, since the in-process cache for the result of getpwnam can be evicted, and the sandbox is blocking access to the Open Directory daemon.

[Per Arne Vollan]

Created attachment 457925 [details]
Patch

[Per Arne Vollan]

Created attachment 457944 [details]
Patch

[Per Arne Vollan]

Created attachment 457945 [details]
Patch

[Per Arne Vollan]

Created attachment 458169 [details]
Patch

[Per Arne Vollan]

Created attachment 458269 [details]
Patch

[Per Arne Vollan]

Created attachment 458276 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/92294145>

[Per Arne Vollan]

<rdar://89758690>

[Darin Adler]

Comment on attachment 458276 [details]
Patch

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

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:824
> +    sandboxExtension->consume();

Why isn’t there code to do this the first time? Or if there is, why doesn’t this share code with it. Seems we also need to do this after process startup time even if the Open Directory cache was not invalidated.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:826
> +    char buffer[4096];

Might need a comment saying why 4096 is the right number to use here.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:827
> +    int bufferSize = sizeof(buffer);

Not sure we need to put this constant into an int. Just call sizeof(buffer) below.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:829
> +    struct passwd pwd;
> +    struct passwd* result = 0;

This is C++, so I think you can write:

    passwd pwd;
    passwd* result = nullptr;

Don’t think we need struct and we don’t need to use 0.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:830
> +    if (getpwuid_r(getuid(), &pwd, buffer, bufferSize, &result) || !result)

It’s incredibly non-obvious that the whole point of this call is to refill some internal caches with accurate data as a side effect, not use the result of this function at all.

I think there needs to be a comment explaining that this is an indirect way to revalidate caches so that calls done to related functions elsewhere in other frameworks get the correct result.

In my opinion it’s a bit fragile to rely on this property of the functions, not documented anywhere I can see, but it may well be necessary to do things this way. Needs a comment.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:831
> +        WTFLogAlways("%s: Couldn't find home directory\n", getprogname());

Do we really need the "\n" here? Doesn’t WTFLog add it?

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:712
> +    m_openDirectoryNotifyTokens.reserveCapacity(WTF_ARRAY_LENGTH(messages));

Can use std::size instead of WTF_ARRAY_LENGTH. We should probably get rid of WTF_ARRAY_LENGTH.

If this is always done exactly once we can use reserveInitialCapacity.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:713
> +    for (unsigned i = 0; i < WTF_ARRAY_LENGTH(messages); i++) {

Range-based for lop is bette for this:

    for (auto* message : messages) {

[Per Arne Vollan]

Created attachment 458374 [details]
Patch

[Per Arne Vollan]

Created attachment 458375 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #9)
> Comment on attachment 458276 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458276&action=review
> 
> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:824
> > +    sandboxExtension->consume();
> 
> Why isn’t there code to do this the first time? Or if there is, why doesn’t
> this share code with it. Seems we also need to do this after process startup
> time even if the Open Directory cache was not invalidated.
> 

Ah, yes, that is a good point. We are currently calling getpwuid_r before entering the sandbox, which does not require the sandbox extension. I have added a new function getHomeDirectory in order to be able to share code.

> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:826
> > +    char buffer[4096];
> 
> Might need a comment saying why 4096 is the right number to use here.
> 

Looking at the man page for getpwuid_r, it seems the correct thing to do is to use the size returned from sysconf(_SC_GETPW_R_SIZE_MAX). So far, I have not changed this, since I believe a size of 4096 should be sufficient. PATH_MAX is 1024 according to sys limits.h. If you'd like, I can change this to use sysconf(_SC_GETPW_R_SIZE_MAX).

> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:827
> > +    int bufferSize = sizeof(buffer);
> 
> Not sure we need to put this constant into an int. Just call sizeof(buffer)
> below.
> 

Fixed!

> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:829
> > +    struct passwd pwd;
> > +    struct passwd* result = 0;
> 
> This is C++, so I think you can write:
> 
>     passwd pwd;
>     passwd* result = nullptr;
> 
> Don’t think we need struct and we don’t need to use 0.
> 

Done!

> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:830
> > +    if (getpwuid_r(getuid(), &pwd, buffer, bufferSize, &result) || !result)
> 
> It’s incredibly non-obvious that the whole point of this call is to refill
> some internal caches with accurate data as a side effect, not use the result
> of this function at all.
> 
> I think there needs to be a comment explaining that this is an indirect way
> to revalidate caches so that calls done to related functions elsewhere in
> other frameworks get the correct result.
> 
> In my opinion it’s a bit fragile to rely on this property of the functions,
> not documented anywhere I can see, but it may well be necessary to do things
> this way. Needs a comment.
> 

Comment added in AuxiliaryProcess::openDirectoryCacheInvalidated.

> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:831
> > +        WTFLogAlways("%s: Couldn't find home directory\n", getprogname());
> 
> Do we really need the "\n" here? Doesn’t WTFLog add it?
> 

Yes, removed "\n".

> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:712
> > +    m_openDirectoryNotifyTokens.reserveCapacity(WTF_ARRAY_LENGTH(messages));
> 
> Can use std::size instead of WTF_ARRAY_LENGTH. We should probably get rid of
> WTF_ARRAY_LENGTH.
> 
> If this is always done exactly once we can use reserveInitialCapacity.
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:713
> > +    for (unsigned i = 0; i < WTF_ARRAY_LENGTH(messages); i++) {
> 
> Range-based for lop is bette for this:
> 
>     for (auto* message : messages) {

Fixed!

Thanks for reviewing!

[Darin Adler]

Comment on attachment 458276 [details]
Patch

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

>>> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:826
>>> +    char buffer[4096];
>> 
>> Might need a comment saying why 4096 is the right number to use here.
> 
> Looking at the man page for getpwuid_r, it seems the correct thing to do is to use the size returned from sysconf(_SC_GETPW_R_SIZE_MAX). So far, I have not changed this, since I believe a size of 4096 should be sufficient. PATH_MAX is 1024 according to sys limits.h. If you'd like, I can change this to use sysconf(_SC_GETPW_R_SIZE_MAX).

I saw that same man page and did not push you to use sysconf because it seemed like overkill given this is Apple-only code and doesn’t need to be portable to super-unusual configurations. On the other hand, I would be OK doing this entirely "by the book". Of course, since we are calling this to trigger a side effect, it’s kind of an "off-label" use anyway. I won’t push for further change, but do think a comment is good.

[Per Arne Vollan]

Created attachment 458379 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #13)
> Comment on attachment 458276 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458276&action=review
> 
> >>> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:826
> >>> +    char buffer[4096];
> >> 
> >> Might need a comment saying why 4096 is the right number to use here.
> > 
> > Looking at the man page for getpwuid_r, it seems the correct thing to do is to use the size returned from sysconf(_SC_GETPW_R_SIZE_MAX). So far, I have not changed this, since I believe a size of 4096 should be sufficient. PATH_MAX is 1024 according to sys limits.h. If you'd like, I can change this to use sysconf(_SC_GETPW_R_SIZE_MAX).
> 
> I saw that same man page and did not push you to use sysconf because it
> seemed like overkill given this is Apple-only code and doesn’t need to be
> portable to super-unusual configurations. On the other hand, I would be OK
> doing this entirely "by the book". Of course, since we are calling this to
> trigger a side effect, it’s kind of an "off-label" use anyway. I won’t push
> for further change, but do think a comment is good.

I uploaded a new patch with a comment.

Thanks for reviewing!

[EWS]

Committed r293509 (?): <https://commits.webkit.org/r293509>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458379 [details].

[Brent Fulgham]

*** Bug 237017 has been marked as a duplicate of this bug. ***