Move all of the fopen() and open() calls to the top of main()
Avoid reusing the 'fd' variable, since it is used read only, then later write only
cap_enter(), and make all except the output FD read/seek only.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Capsicum part look's fine for me. Thanks, for that.
I have a lot of issue with the style of file. But maybe we should keep it in the same way?
| usr.bin/bsdiff/bspatch/bspatch.c | ||
|---|---|---|
| 242 ↗ | (On Diff #18865) | You changing style of the file: |
Comment Actions
Well, the file is already a mix and match of style(9) and non-style(9). My suggestion would be to fix up any code to be properly formatted if you're changing it anyway.
Allan: Thanks for working on this!
| usr.bin/bsdiff/bspatch/bspatch.c | ||
|---|---|---|
| 38 ↗ | (On Diff #18865) | Could you move this above the other includes? Everything for <sys/*> should go before <*>. |
| 102 ↗ | (On Diff #18865) | Please add spaces between these O_* flags: O_RDONLY | O_BINARY. Same below. Maybe just axe the O_BINARY stuff? That's only useful if we wanted to make this work on Windows. |
| 182 ↗ | (On Diff #18865) | While you're modifying these lines, could you please add missing whitespace and put err() on a separate line? |
| 242 ↗ | (On Diff #18865) | Which is fine, while you're at it. Could you also remove the extraneous parentheses here? |
| usr.bin/bsdiff/bspatch/bspatch.c | ||
|---|---|---|
| 93 ↗ | (On Diff #18865) | Why the same file needs to be opened so many times? |
| 105 ↗ | (On Diff #18865) | The file should be removed if we hit an error. |
| 108 ↗ | (On Diff #18865) | This means users without Capsicum kernel support would not be able to run this at all. Can't this be changed to fallback mechanisms, either refuse run as root, or chroot to /var/empty then drop all privileges? |
| usr.bin/bsdiff/bspatch/bspatch.c | ||
|---|---|---|
| 105 ↗ | (On Diff #18865) | if there is an error on open, is the file created? |
| usr.bin/bsdiff/bspatch/bspatch.c | ||
|---|---|---|
| 38 ↗ | (On Diff #19595) | I just wonder do we need to check this? |
| 84 ↗ | (On Diff #19595) | #ifdef HAVE_CAPSICUM? |
| 124 ↗ | (On Diff #19595) | You need to setuid this binary, I think, so you need as well do some setgid/setuid/setgroup. |
| 148 ↗ | (On Diff #19595) | All that in ifdef. |
| usr.bin/bsdiff/bspatch/bspatch.c | ||
|---|---|---|
| 38 ↗ | (On Diff #19595) | We're upstream for this file and it may end up being copied into other projects, so there's some argument for keeping a version check. |
| 119 ↗ | (On Diff #19595) | If we HAVE_CAPSICUM then cap_enter() < 0 with errno != ENOSYS should be fatal. |
| 124 ↗ | (On Diff #19595) | I'd prefer that we leave this out of the initial change. |
| 137–144 ↗ | (On Diff #19595) | I think this is cleaner as if ((cap_rights_limit(fileno(f), &rights_ro) < 0 && errno != ENOSYS) ||
(cap_rights_limit(fileno(cpf), &rights_ro) < 0 && errno != ENOSYS) ||
(cap_rights_limit(fileno(dpf), &rights_ro) < 0 && errno != ENOSYS) ||
(cap_rights_limit(fileno(epf), &rights_ro) < 0 && errno != ENOSYS))
err(1, ... |
| 270–271 ↗ | (On Diff #19595) | Leftover |
| 93 ↗ | (On Diff #18865) | Agree that it's a bit unusual, but it's already like this and I'd rather see a minimal change to capsicumize it. |
| 105 ↗ | (On Diff #18865) | If there's an error later on (e.g. malloc, BZ2_bzReadOpen etc.) the temp newfile will be left behind. |
| 108 ↗ | (On Diff #18865) | In my opinion we should look at such a change in a subsequent patch. |
| usr.bin/bsdiff/bspatch/bspatch.c | ||
|---|---|---|
| 124 ↗ | (On Diff #19595) | Yes, leave anything to do with chroot out of this change. We have capsicum in FreeBSD, and I don't want to risk introducing other errors by developing the chroot, suid, etc. support in our tree at the same time. IMO that work would best be developed in a standalone repository, but it could be brought back when done to simplify ongoing maintenance. |
| 105 ↗ | (On Diff #18865) | That said, I'd be happy with the capsicumization going in now, and addressing the leftover file in a subsequent commit. |
| usr.bin/bsdiff/bspatch/bspatch.c | ||
|---|---|---|
| 126 ↗ | (On Diff #19608) | In my opinion we should not inform user about that. The same case for platforms that are not build with CAPABILITIEY per default (like arm), there is no point to inform about that. |
Comment Actions
LGTM in general, thanks for working on this one. Please fix the two style issues pointed out by Ed when committing.