17 apr. 2021 kl. 21.42 skrev Philipp : >> --- a/lisp/subr.el > > Should this maybe be in a platform-specific file such as ns-fns.el (like dos-fns.el, w32-fns.el)? Yes, and thank you for noticing. Now moved. > I don't think we use the "darwin-" prefix anywhere else in Emacs. Maybe use a "ns-" prefix. As Alan correctly noted this has nothing with NS to do per se, so I'm staying with darwin- for now. > Also I think such internal functions commonly have an "internal" piece somewhere in their name, e.g. "ns-enter-sandbox-internal". Maybe, but there is already a platform-specific prefix and a clear note in the doc string that it's internal. Doesn't that suffice? >> + (darwin-sandbox-init > > Can you also refer to the documents listed below, so that readers aren't wondering what this syntax means? Thank you but that sounds unlikely since the PROFILE argument is described as SBPL in the function doc string. >> + (concat "(version 1)\n" > > Since this uses Lisp syntax, maybe use (prin1-to-string `(...))? I'd rather not, since it's not exactly Emacs Lisp syntax. I'd like to avoiding conflating the two as far as possible. However... >> + (format "(allow file-read* (subpath %S))\n" dir)) > > Are you sure that the string quoting syntaxes are compatible? It had me concerned when I wrote it too but it didn't seem possible to cause a false positive that way -- false negatives (access denial) at worst. In particular, names containing backslashes or double quotes work correctly. >> + doc: /* Enter a sandbox whose permitted access is curtailed by PROFILE. >> +Already open descriptors can be used freely. >> +PROFILE is a string in the macOS sandbox profile language, >> +a set of rules in a Lisp-like syntax. > > I'd also refer to the Chromium document here, otherwise C-h f for this function won't be very useful. I wouldn't worry -- an Emacs developer who doesn't already know it is more likely to duckduckgo "macos sandbox profile language" and get an up-to-date reference. >> + if (sandbox_init_with_parameters (SSDATA (profile), 0, NULL, &err) != 0) > > If you're using SSDATA, better check that the string doesn't contain embedded null bytes. There is really no way that that could ever be a problem but I added a check just in case. > Also does this need to be encoded somehow? What happens if the string contains non-Unicode characters like raw bytes? Then there will be false negatives (permissions not granted) or syntax errors. This is just a system call wrapper; the caller is responsible for using reasonable arguments. >> + error ("sandbox error: %s", err); > > This looks like an error that clients could handle, so I'd signal a specific error symbol here. That error just indicates a programming mistake. Nobody is going to handle it. >> diff --git a/test/src/emacs-tests.el b/test/src/emacs-tests.el > > This should probably be in subr-tests.el since it tests a function in subr.el. Right again, moved to a new file. > I like tests that are somewhat repetitive but more decoupled and easier to read better than tests with factored-out assertions. There is merit to such a style but the more important concern in this case is to avoid false positives and negatives, and make the tests robust in that regard. It is very easy to make a mistake that renders a test meaningless, especially when testing a mechanism that does not alter the value of a computation but merely allows it to proceed or causes it to fail. The test has now been rewritten in a kind of oracular-combinatorial style which is effective in these situations. Doing so also improved coverage. Thank you very much for your review and comments! I'd like to push the resulting patch but perhaps it and the rest of the sandbox development should go into a separate branch?