[Webkit-unassigned] [Bug 239513] [macOS] The function getpwnam can sometimes fail

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=239513

--- Comment #12 from Per Arne Vollan <pvollan at apple.com> ---
(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!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220426/3601b71d/attachment-0001.htm>