Differential D7358 Diff 19595 usr.bin/bsdiff/bspatch/bspatch.c

Changeset View

Standalone View

usr.bin/bsdiff/bspatch/bspatch.c

Show All 21 Lines
* STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING		* STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
* IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE		* IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.		* POSSIBILITY OF SUCH DAMAGE.
*/		*/

#include <sys/cdefs.h>		#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");		__FBSDID("$FreeBSD$");

		#if defined(__FreeBSD__)
		emasteUnsubmitted Not Done Inline Actions it could go in the `#if defined(__FreeBSD__)` block if you like since it's not needed on !FreeBSD emaste: it could go in the `#if defined(__FreeBSD__)` block if you like since it's not needed on !
		#if __FreeBSD_version >= 1100014
		emasteUnsubmitted Not Done Inline Actions need to add `#include <sys/param.h>` to pick up __FreeBSD_version emaste: need to add `#include <sys/param.h>` to pick up __FreeBSD_version
		#include <sys/capsicum.h>
		#define HAVE_CAPSICUM
		#elif __FreeBSD_version >= 1000000
		#include <sys/capability.h>
		#define HAVE_CAPSICUM
		#endif
		#endif
		oshogboUnsubmitted Not Done Inline Actions I just wonder do we need to check this? In my opinion we should include <sys/capsicum.h> and if we would merge it in feature to 10 we could change the header just in that version, right? oshogbo: I just wonder do we need to check this? In my opinion we should include <sys/capsicum.h> and if…
		emasteUnsubmitted Not Done Inline Actions 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. emaste: We're upstream for this file and it may end up being copied into other projects, so there's…

#include <bzlib.h>		#include <bzlib.h>
#include <err.h>		#include <err.h>
		#include <errno.h>
#include <fcntl.h>		#include <fcntl.h>
#include <stdio.h>		#include <stdio.h>
#include <stdlib.h>		#include <stdlib.h>
#include <string.h>		#include <string.h>
#include <unistd.h>		#include <unistd.h>

#ifndef O_BINARY		#ifndef O_BINARY
		edUnsubmitted Not Done Inline Actions Could you move this above the other includes? Everything for `<sys/>` should go before `<>`. ed: Could you move this above the other includes? Everything for `<sys/>` should go before `<>`.
#define O_BINARY 0		#define O_BINARY 0
#endif		#endif

static off_t offtin(u_char *buf)		static off_t offtin(u_char *buf)
{		{
off_t y;		off_t y;

y = buf[7] & 0x7F;		y = buf[7] & 0x7F;
Show All 18 Lines	usage(void)
fprintf(stderr, "usage: bspatch oldfile newfile patchfile\n");		fprintf(stderr, "usage: bspatch oldfile newfile patchfile\n");
exit(1);		exit(1);
}		}

int main(int argc, char *argv[])		int main(int argc, char *argv[])
{		{
FILE f, cpf, dpf, epf;		FILE f, cpf, dpf, epf;
BZFILE cpfbz2, dpfbz2, *epfbz2;		BZFILE cpfbz2, dpfbz2, *epfbz2;
		cap_rights_t rights_ro, rights_wr;
		oshogboUnsubmitted Not Done Inline Actions #ifdef HAVE_CAPSICUM? oshogbo: #ifdef HAVE_CAPSICUM?
int cbz2err, dbz2err, ebz2err;		int cbz2err, dbz2err, ebz2err;
int newfd, oldfd;		int newfd, oldfd;
ssize_t oldsize, newsize;		ssize_t oldsize, newsize;
ssize_t bzctrllen, bzdatalen;		ssize_t bzctrllen, bzdatalen;
u_char header[32], buf[8];		u_char header[32], buf[8];
u_char old, new;		u_char old, new;
off_t oldpos, newpos;		off_t oldpos, newpos;
off_t ctrl[3];		off_t ctrl[3];
off_t lenread;		off_t lenread;
off_t i;		off_t i;

if (argc != 4)		if (argc != 4)
usage();		usage();

/* Open patch file */		/* Open patch file */
if ((f = fopen(argv[3], "rb")) == NULL)		if ((f = fopen(argv[3], "rb")) == NULL)
err(1, "fopen(%s)", argv[3]);		err(1, "fopen(%s)", argv[3]);
		/* Open patch file for control block */
		if ((cpf = fopen(argv[3], "rb")) == NULL)
		delphijUnsubmitted Not Done Inline Actions Why the same file needs to be opened so many times? delphij: Why the same file needs to be opened so many times?
		emasteUnsubmitted Not Done Inline Actions Agree that it's a bit unusual, but it's already like this and I'd rather see a minimal change to capsicumize it. emaste: Agree that it's a bit unusual, but it's already like this and I'd rather see a minimal change…
		err(1, "fopen(%s)", argv[3]);
		/* open patch file for diff block */
		if ((dpf = fopen(argv[3], "rb")) == NULL)
		err(1, "fopen(%s)", argv[3]);
		/* open patch file for extra block */
		if ((epf = fopen(argv[3], "rb")) == NULL)
		err(1, "fopen(%s)", argv[3]);
		/* open oldfile */
		if ((oldfd = open(argv[1], O_RDONLY \| O_BINARY, 0)) < 0)
		edUnsubmitted Not Done Inline Actions 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. ed:* Please add spaces between these `O_*` flags: `O_RDONLY \| O_BINARY`. Same below. Maybe just axe…
		err(1,"open(%s)", argv[1]);
		emasteUnsubmitted Not Done Inline Actions space after , emaste: space after ,
		/* open newfile */
		if ((newfd = open(argv[2], O_CREAT \| O_TRUNC \| O_WRONLY \| O_BINARY, 0666)) < 0)
		delphijUnsubmitted Not Done Inline Actions The file should be removed if we hit an error. delphij: The file should be removed if we hit an error.
		allanjudeAuthorUnsubmitted Not Done Inline Actions if there is an error on open, is the file created? allanjude: if there is an error on open, is the file created?
		emasteUnsubmitted Not Done Inline Actions If there's an error later on (e.g. `malloc`, `BZ2_bzReadOpen` etc.) the temp newfile will be left behind. emaste: If there's an error later on (e.g. `malloc`, `BZ2_bzReadOpen` etc.) the temp newfile will be…
		emasteUnsubmitted Not Done Inline Actions That said, I'd be happy with the capsicumization going in now, and addressing the leftover file in a subsequent commit. emaste: That said, I'd be happy with the capsicumization going in now, and addressing the leftover file…
		emasteUnsubmitted Not Done Inline Actions Overly long line? emaste: Overly long line?
		err(1,"open(%s)", argv[2]);

		emasteUnsubmitted Not Done Inline Actions space after , emaste: space after ,
		#ifdef HAVE_CAPSICUM
		delphijUnsubmitted Not Done Inline Actions 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? delphij: This means users without Capsicum kernel support would not be able to run this at all. Can't…
		emasteUnsubmitted Not Done Inline Actions In my opinion we should look at such a change in a subsequent patch. emaste: In my opinion we should look at such a change in a subsequent patch.
		if (cap_enter() < 0) {
		emasteUnsubmitted Not Done Inline Actions If we `HAVE_CAPSICUM` then `cap_enter() < 0` with `errno != ENOSYS` should be fatal. emaste: If we `HAVE_CAPSICUM` then `cap_enter() < 0` with `errno != ENOSYS` should be fatal.
		/* Failed to engauge Capsicum */
		emasteUnsubmitted Not Done Inline Actions engage emaste: engage
		#else
		{
		oshogboUnsubmitted Not Done Inline Actions In my opinion we should not inform user about that. He did not compile capsicum in to the system, it was his decision we should not inform go on every step that he don't have sandbox. He already made his choices. The same case for platforms that are not build with CAPABILITIEY per default (like arm), there is no point to inform about that. oshogbo: In my opinion we should not inform user about that. He did not compile capsicum in to the…
		#endif
		if (chroot("/var/empty") < 0) {
		oshogboUnsubmitted Not Done Inline Actions You need to setuid this binary, I think, so you need as well do some setgid/setuid/setgroup. Otherwise I think you are escalating user privileged. oshogbo: You need to setuid this binary, I think, so you need as well do some setgid/setuid/setgroup.
		emasteUnsubmitted Not Done Inline Actions I'd prefer that we leave this out of the initial change. emaste: I'd prefer that we leave this out of the initial change.
		oshogboUnsubmitted Not Done Inline Actions If you mean idea of chrooting then I agree. Sandboxing which chroot will add you a lot of more work. As well force all other project to suid this binary. oshogbo: If you mean idea of chrooting then I agree. Sandboxing which chroot will add you a lot of more…
		emasteUnsubmitted Not Done Inline Actions 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. emaste: Yes, leave anything to do with chroot out of this change. We have capsicum in FreeBSD, and I…
		allanjudeAuthorUnsubmitted Not Done Inline Actions the plan with chroot was to just emit the warning and NOT do it if run as root. Definitely do NOT want to suid this binary. Will leave the chroot part out for now allanjude: the plan with chroot was to just emit the warning and NOT do it if run as root. Definitely do…
		/* Skip the security sandbox if run as non-root */
		if (errno == EPERM) {
		warn("No security sandbox availabile");
		} else {
		err(1, "failed to enter security sandbox");
		}
		}
		}

		cap_rights_init(&rights_ro, CAP_READ, CAP_FSTAT, CAP_SEEK);
		cap_rights_init(&rights_wr, CAP_WRITE);

		if (cap_rights_limit(fileno(f), &rights_ro) < 0)
		err(1, "cap_rights_limit() failed on patch");
		if (cap_rights_limit(fileno(cpf), &rights_ro) < 0)
		err(1, "cap_rights_limit() failed on patch");
		if (cap_rights_limit(fileno(dpf), &rights_ro) < 0)
		err(1, "cap_rights_limit() failed on patch");
		if (cap_rights_limit(fileno(epf), &rights_ro) < 0)
		err(1, "cap_rights_limit() failed on patch");
		emasteUnsubmitted Not Done Inline Actions 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, ... emaste: I think this is cleaner as ``` if ((cap_rights_limit(fileno(f), &rights_ro) < 0 && errno !=…
		if (cap_rights_limit(oldfd, &rights_ro) < 0)
		err(1, "cap_rights_limit() failed on input");
		if (cap_rights_limit(newfd, &rights_wr) < 0)
		err(1, "cap_rights_limit() failed on output");
		oshogboUnsubmitted Not Done Inline Actions All that in ifdef. oshogbo: All that in ifdef.

/*		/*
File format:		File format:
0 8 "BSDIFF40"		0 8 "BSDIFF40"
8 8 X		8 8 X
16 8 Y		16 8 Y
24 8 sizeof(newfile)		24 8 sizeof(newfile)
32 X bzip2(control block)		32 X bzip2(control block)
32+X Y bzip2(diff block)		32+X Y bzip2(diff block)
Show All 19 Lines	#endif
bzdatalen = offtin(header + 16);		bzdatalen = offtin(header + 16);
newsize = offtin(header + 24);		newsize = offtin(header + 24);
if ((bzctrllen < 0) \|\| (bzdatalen < 0) \|\| (newsize < 0))		if ((bzctrllen < 0) \|\| (bzdatalen < 0) \|\| (newsize < 0))
errx(1, "Corrupt patch\n");		errx(1, "Corrupt patch\n");

/* Close patch file and re-open it via libbzip2 at the right places */		/* Close patch file and re-open it via libbzip2 at the right places */
if (fclose(f))		if (fclose(f))
err(1, "fclose(%s)", argv[3]);		err(1, "fclose(%s)", argv[3]);
if ((cpf = fopen(argv[3], "rb")) == NULL)
err(1, "fopen(%s)", argv[3]);
if (fseeko(cpf, 32, SEEK_SET))		if (fseeko(cpf, 32, SEEK_SET))
err(1, "fseeko(%s, %lld)", argv[3],		err(1, "fseeko(%s, %lld)", argv[3],
(long long)32);		(long long)32);
if ((cpfbz2 = BZ2_bzReadOpen(&cbz2err, cpf, 0, 0, NULL, 0)) == NULL)		if ((cpfbz2 = BZ2_bzReadOpen(&cbz2err, cpf, 0, 0, NULL, 0)) == NULL)
errx(1, "BZ2_bzReadOpen, bz2err = %d", cbz2err);		errx(1, "BZ2_bzReadOpen, bz2err = %d", cbz2err);
if ((dpf = fopen(argv[3], "rb")) == NULL)
err(1, "fopen(%s)", argv[3]);
if (fseeko(dpf, 32 + bzctrllen, SEEK_SET))		if (fseeko(dpf, 32 + bzctrllen, SEEK_SET))
err(1, "fseeko(%s, %lld)", argv[3],		err(1, "fseeko(%s, %lld)", argv[3],
(long long)(32 + bzctrllen));		(long long)(32 + bzctrllen));
if ((dpfbz2 = BZ2_bzReadOpen(&dbz2err, dpf, 0, 0, NULL, 0)) == NULL)		if ((dpfbz2 = BZ2_bzReadOpen(&dbz2err, dpf, 0, 0, NULL, 0)) == NULL)
errx(1, "BZ2_bzReadOpen, bz2err = %d", dbz2err);		errx(1, "BZ2_bzReadOpen, bz2err = %d", dbz2err);
if ((epf = fopen(argv[3], "rb")) == NULL)
err(1, "fopen(%s)", argv[3]);
if (fseeko(epf, 32 + bzctrllen + bzdatalen, SEEK_SET))		if (fseeko(epf, 32 + bzctrllen + bzdatalen, SEEK_SET))
err(1, "fseeko(%s, %lld)", argv[3],		err(1, "fseeko(%s, %lld)", argv[3],
(long long)(32 + bzctrllen + bzdatalen));		(long long)(32 + bzctrllen + bzdatalen));
if ((epfbz2 = BZ2_bzReadOpen(&ebz2err, epf, 0, 0, NULL, 0)) == NULL)		if ((epfbz2 = BZ2_bzReadOpen(&ebz2err, epf, 0, 0, NULL, 0)) == NULL)
errx(1, "BZ2_bzReadOpen, bz2err = %d", ebz2err);		errx(1, "BZ2_bzReadOpen, bz2err = %d", ebz2err);

oldfd = open(argv[1], O_RDONLY \| O_BINARY, 0);
if (oldfd < 0)		if (oldfd < 0)
err(1, "%s", argv[1]);		err(1, "%s", argv[1]);
		edUnsubmitted Not Done Inline Actions While you're modifying these lines, could you please add missing whitespace and put `err()` on a separate line? ed: While you're modifying these lines, could you please add missing whitespace and put `err()` on…
		allanjudeAuthorUnsubmitted Not Done Inline Actions also leftover allanjude: also leftover
if ((oldsize = lseek(oldfd, 0, SEEK_END)) == -1 \|\|		if ((oldsize = lseek(oldfd, 0, SEEK_END)) == -1 \|\|
(old = malloc(oldsize+1)) == NULL \|\|		(old = malloc(oldsize+1)) == NULL \|\|
lseek(oldfd, 0, SEEK_SET) != 0 \|\|		lseek(oldfd, 0, SEEK_SET) != 0 \|\|
read(oldfd, old, oldsize) != oldsize \|\|		read(oldfd, old, oldsize) != oldsize \|\|
close(oldfd) == -1)		close(oldfd) == -1)
err(1, "%s", argv[1]);		err(1, "%s", argv[1]);
if ((new = malloc(newsize + 1)) == NULL)		if ((new = malloc(newsize + 1)) == NULL)
err(1, NULL);		err(1, NULL);
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines	#endif

/* Clean up the bzip2 reads */		/* Clean up the bzip2 reads */
BZ2_bzReadClose(&cbz2err, cpfbz2);		BZ2_bzReadClose(&cbz2err, cpfbz2);
BZ2_bzReadClose(&dbz2err, dpfbz2);		BZ2_bzReadClose(&dbz2err, dpfbz2);
BZ2_bzReadClose(&ebz2err, epfbz2);		BZ2_bzReadClose(&ebz2err, epfbz2);
if (fclose(cpf) \|\| fclose(dpf) \|\| fclose(epf))		if (fclose(cpf) \|\| fclose(dpf) \|\| fclose(epf))
err(1, "fclose(%s)", argv[3]);		err(1, "fclose(%s)", argv[3]);

/* Write the new file */		/* Write the new file */
newfd = open(argv[2], O_CREAT \| O_TRUNC \| O_WRONLY \| O_BINARY, 0666);
if (newfd < 0)		if (newfd < 0)
		oshogboUnsubmitted Not Done Inline Actions You changing style of the file: if ((write(wfd, new, newsize)!=newsize) \|\| (close(wfd)==-1)) oshogbo: You changing style of the file: if ((write(wfd, new, newsize)!=newsize) \|\| (close(wfd)==-1))
		edUnsubmitted Not Done Inline Actions Which is fine, while you're at it. Could you also remove the extraneous parentheses here? ed: Which is fine, while you're at it. Could you also remove the extraneous parentheses here?
err(1, "%s", argv[2]);		err(1, "%s", argv[2]);
		emasteUnsubmitted Not Done Inline Actions Leftover emaste: Leftover
if (write(newfd, new, newsize) != newsize \|\| close(newfd) == -1)		if (write(newfd, new, newsize) != newsize \|\| close(newfd) == -1)
err(1, "%s", argv[2]);		err(1, "%s", argv[2]);

free(new);		free(new);
free(old);		free(old);

return (0);		return (0);
}		}