Thank you for the replies Ingo and the diffs!
George Brown
On 26 August 2017 at 17:04, Ingo Schwarze <schwa...@usta.de> wrote:
> Hi George,
>
> George Brown wrote on Thu, Aug 24, 2017 at 02:01:05PM +0100:
>
>> In mandocdb.c it appears cmp(1) and rm(1) are executed in a child
>> process. It seems that if the logic from these programs were duplicated
>> the pledge in mandocdb.c could be further restricted and even not bother
>> with forking.
>
> Done as well, see the commit below.
>
> Thanks again for the suggestion,
> Ingo
>
>
> Log Message:
> -----------
> Do not fork and exec cmp(1); instead, simply fstat(2), mmap(2), and
> compare the files directly, allowing a much stricter pledge(2), at
> very little cost: merely 15 additional lines of very simple code.
> Suggested by George Brown <321 dot george at gmail dot com> on misc@.
>
> Modified Files:
> --------------
> mandoc:
> mandocdb.c
>
> Revision Data
> -------------
> Index: mandocdb.c
> ===================================================================
> RCS file: /home/cvs/mandoc/mandoc/mandocdb.c,v
> retrieving revision 1.254
> retrieving revision 1.255
> diff -Lmandocdb.c -Lmandocdb.c -u -p -r1.254 -r1.255
> --- mandocdb.c
> +++ mandocdb.c
> @@ -19,8 +19,8 @@
> #include "config.h"
>
> #include <sys/types.h>
> +#include <sys/mman.h>
> #include <sys/stat.h>
> -#include <sys/wait.h>
>
> #include <assert.h>
> #include <ctype.h>
> @@ -319,7 +319,7 @@ mandocdb(int argc, char *argv[])
> int ch, i;
>
> #if HAVE_PLEDGE
> - if (pledge("stdio rpath wpath cpath fattr flock proc exec", NULL) ==
> -1) {
> + if (pledge("stdio rpath wpath cpath", NULL) == -1) {
> warn("pledge");
> return (int)MANDOCLEVEL_SYSERR;
> }
> @@ -440,15 +440,6 @@ mandocdb(int argc, char *argv[])
> * The existing database is usable. Process
> * all files specified on the command-line.
> */
> -#if HAVE_PLEDGE
> - if (!nodb) {
> - if (pledge("stdio rpath wpath cpath fattr
> flock", NULL) == -1) {
> - warn("pledge");
> - exitcode = (int)MANDOCLEVEL_SYSERR;
> - goto out;
> - }
> - }
> -#endif
> use_all = 1;
> for (i = 0; i < argc; i++)
> filescan(argv[i]);
> @@ -2119,9 +2110,10 @@ dbprune(struct dba *dba)
> static void
> dbwrite(struct dba *dba)
> {
> - char tfn[33];
> - int status;
> - pid_t child;
> + struct stat sb1, sb2;
> + char tfn[33], *cp1, *cp2;
> + off_t i;
> + int fd1, fd2;
>
> /*
> * Do not write empty databases, and delete existing ones
> @@ -2160,39 +2152,59 @@ dbwrite(struct dba *dba)
> say("", "&%s", tfn);
> return;
> }
> -
> + cp1 = cp2 = NULL;
> + fd1 = fd2 = -1;
> (void)strlcat(tfn, "/" MANDOC_DB, sizeof(tfn));
> if (dba_write(tfn, dba) == -1) {
> - exitcode = (int)MANDOCLEVEL_SYSERR;
> say(tfn, "&dba_write");
> - goto out;
> - }
> -
> - switch (child = fork()) {
> - case -1:
> - exitcode = (int)MANDOCLEVEL_SYSERR;
> - say("", "&fork cmp");
> - return;
> - case 0:
> - execlp("cmp", "cmp", "-s", tfn, MANDOC_DB, (char *)NULL);
> - say("", "&exec cmp");
> - exit(0);
> - default:
> - break;
> - }
> - if (waitpid(child, &status, 0) == -1) {
> - exitcode = (int)MANDOCLEVEL_SYSERR;
> - say("", "&wait cmp");
> - } else if (WIFSIGNALED(status)) {
> - exitcode = (int)MANDOCLEVEL_SYSERR;
> - say("", "cmp died from signal %d", WTERMSIG(status));
> - } else if (WEXITSTATUS(status)) {
> - exitcode = (int)MANDOCLEVEL_SYSERR;
> - say(MANDOC_DB,
> - "Data changed, but cannot replace database");
> + goto err;
> }
> + if ((fd1 = open(MANDOC_DB, O_RDONLY, 0)) == -1) {
> + say(MANDOC_DB, "&open");
> + goto err;
> + }
> + if ((fd2 = open(tfn, O_RDONLY, 0)) == -1) {
> + say(tfn, "&open");
> + goto err;
> + }
> + if (fstat(fd1, &sb1) == -1) {
> + say(MANDOC_DB, "&fstat");
> + goto err;
> + }
> + if (fstat(fd2, &sb2) == -1) {
> + say(tfn, "&fstat");
> + goto err;
> + }
> + if (sb1.st_size != sb2.st_size)
> + goto err;
> + if ((cp1 = mmap(NULL, sb1.st_size, PROT_READ, MAP_PRIVATE,
> + fd1, 0)) == NULL) {
> + say(MANDOC_DB, "&mmap");
> + goto err;
> + }
> + if ((cp2 = mmap(NULL, sb2.st_size, PROT_READ, MAP_PRIVATE,
> + fd2, 0)) == NULL) {
> + say(tfn, "&mmap");
> + goto err;
> + }
> + for (i = 0; i < sb1.st_size; i++)
> + if (cp1[i] != cp2[i])
> + goto err;
> + goto out;
> +
> +err:
> + exitcode = (int)MANDOCLEVEL_SYSERR;
> + say(MANDOC_DB, "Data changed, but cannot replace database");
>
> out:
> + if (cp1 != NULL)
> + munmap(cp1, sb1.st_size);
> + if (cp2 != NULL)
> + munmap(cp2, sb2.st_size);
> + if (fd1 != -1)
> + close(fd1);
> + if (fd2 != -1)
> + close(fd2);
> unlink(tfn);
> *strrchr(tfn, '/') = '\0';
> rmdir(tfn);
