[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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com
 Attachment #458276|review?                     |review+
              Flags|                            |

--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 458276
  --> https://bugs.webkit.org/attachment.cgi?id=458276
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) {

-- 
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/20220425/811f73d8/attachment.htm>