Page MenuHomeFreeBSD

capsicum bspatch(1)
AbandonedPublic

Authored by emaste on Aug 23 2016, 2:46 AM.

Details

Reviewers
cperciva
oshogbo
Summary

Refactor to first open all fds and FILE *s, and move processing after cap_enter.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

emaste retitled this revision from to capsicum bspatch(1).
emaste updated this object.
emaste edited the test plan for this revision. (Show Details)
emaste added a reviewer: cperciva.

I'm not sure if you spoke with AllanJude but he proposed patch https://reviews.freebsd.org/D7358 for bspatch .
Capsicumizing looks good for me.

There was a general request in that patch: "People downstream bspatch in other projects. Can we ifdef this based on something?", in tcpdump we ifdef it based on FreeBSD.

usr.bin/bsdiff/bspatch/bspatch.c
29

sys should be in separate block, right?

#include <sys/capsicum.h>

#include <...>
132

I'm not sure about style of this file.
If we want be correct with style(9) then I think it should be:
open(argv[1], O_RDONLY | O_BINARY, 0);

The same with:
open(argv[2], O_CREAT | O_TRUNC | O_WRONLY | O_BINARY, 0666);

135

delphij@ pointed out in original patch, that this file should be removed if any other error accrues.

Oh, I wasn't aware of D7358 - I'll discuss with @allanjude.

usr.bin/bsdiff/bspatch/bspatch.c
132

Yes, the style in here is already a mess, but you're right that these should be fixed.

In fact I think we should perhaps first style(9) this file (or at least the parts we're going to be changing -- file opening, seeking etc.), to make the subsequent work for capsicum more clear.

135

Yes, indeed.

Yes, we can #ifdef it easily. For elftoolchain I proposed this:

https://github.com/emaste/elftoolchain/commit/ce3c99c295001084aaaa7da29770797f3b268fa9#diff-0b00df06b17cf879b89570db09feec5eR48

not sure if we should bother with the version checks in-tree.

Yes, we can #ifdef it easily. For elftoolchain I proposed this:

https://github.com/emaste/elftoolchain/commit/ce3c99c295001084aaaa7da29770797f3b268fa9#diff-0b00df06b17cf879b89570db09feec5eR48

not sure if we should bother with the version checks in-tree.

I like your ifdef style there.

I also like delphij's idea of, in the case of there not being capsicum, chroot to /var/empty, but this requires root. I have a modified version of my patch that tries to address this, but it started to get ugly. I can post it later today

emaste edited edge metadata.
  • fix typo spotted by @allanjude
  • remove unnecessary brackets

oops, I updated the wrong review :(