On Wed, 2018-11-28 at 08:20 +0700, Robert Elz wrote: > Date: Wed, 28 Nov 2018 00:23:47 +0100 > From: =?UTF-8?Q?Micha=C5=82_G=C3=B3rny?= <mgo...@gentoo.org> > Message-ID: <1543361027.17222.11.ca...@gentoo.org> > > I have no idea whether this is something that we want or not, > but assuming that we do ... > > | so I'd appreciate some pointers if I'm doing things right. > > I think you need to watch for sign extension from *str (str is a char *, not > u_char *) which probably means &0xFF is needed, and I'd suggest > that rather than doing <<8 >> 8 and comparing for equality, testing > whether you're going to overflow when doing a <<8 is better done as > if (tval > (UINTMAX_MAX/256)) > > I also see that the other conversions work if len == 0 (they return 0), > which your one doesn't, it might be worth a "len > 0" in the initial test > (so you never look at *str if len == 0).
Thank you for your comments. FTR, I don't think those functions can
even take len==0 (given they're passed only positive values by design)
but I suppose there's no harm in checking for it explicitly.
I've also changed the test case to check whether *str >= 0x80 works
correctly.
Here's the updated patch:
Index: bin/pax/gen_subs.c
===================================================================
RCS file: /pub/NetBSD-CVS/src/bin/pax/gen_subs.c,v
retrieving revision 1.36
diff -u -B -d -u -p -r1.36 gen_subs.c
--- bin/pax/gen_subs.c 9 Aug 2012 08:09:21 -0000 1.36
+++ bin/pax/gen_subs.c 28 Nov 2018 17:23:45 -0000
@@ -306,12 +306,10 @@ u32_asc(uintmax_t val, char *str, int le
/*
* asc_umax()
- * convert hex/octal character string into a uintmax. We do
- * not have to to check for overflow! (the headers in all
supported
- * formats are not large enough to create an overflow).
+ * convert hex/octal/base-256 value into a uintmax.
* NOTE: strings passed to us are NOT TERMINATED.
* Return:
- * uintmax_t value
+ * uintmax_t value; UINTMAX_MAX for overflow/negative
*/
uintmax_t
@@ -323,6 +321,30 @@ asc_umax(char *str, int len, int base)
stop = str + len;
/*
+ * if the highest bit of first byte is set, it's base-256 encoded
+ * (base-256 is basically (n-1)-bit big endian signed
+ */
+ if (str < stop && (*str & 0x80)) {
+ /*
+ * uintmax_t can't be negative, so fail on negative numbers
+ */
+ if (*str & 0x40)
+ return UINTMAX_MAX;
+
+ tval = *str++ & 0x3f;
+ while (str < stop) {
+ /*
+ * check for overflow
+ */
+ if (tval > (UINTMAX_MAX/256))
+ return UINTMAX_MAX;
+ tval = (tval << 8) | ((*str++) & 0xFF);
+ }
+
+ return tval;
+ }
+
+ /*
* skip over leading blanks and zeros
*/
while ((str < stop) && ((*str == ' ') || (*str == '0')))
Index: bin/pax/tar.c
===================================================================
RCS file: /pub/NetBSD-CVS/src/bin/pax/tar.c,v
retrieving revision 1.73
diff -u -B -d -u -p -r1.73 tar.c
--- bin/pax/tar.c 19 Dec 2015 18:28:54 -0000 1.73
+++ bin/pax/tar.c 28 Nov 2018 17:23:45 -0000
@@ -486,6 +486,8 @@ tar_rd(ARCHD *arcn, char *buf)
arcn->sb.st_uid = (uid_t)asc_u32(hd->uid, sizeof(hd->uid),
OCT);
arcn->sb.st_gid = (gid_t)asc_u32(hd->gid, sizeof(hd->gid), OCT);
arcn->sb.st_size = (off_t)ASC_OFFT(hd->size, sizeof(hd->size), OCT);
+ if (arcn->sb.st_size == -1)
+ return -1;
arcn->sb.st_mtime = (time_t)(int32_t)asc_u32(hd->mtime,
sizeof(hd->mtime), OCT);
arcn->sb.st_ctime = arcn->sb.st_atime = arcn->sb.st_mtime;
@@ -860,6 +862,8 @@ ustar_rd(ARCHD *arcn, char *buf)
arcn->sb.st_mode = (mode_t)(asc_u32(hd->mode, sizeof(hd->mode), OCT) &
0xfff);
arcn->sb.st_size = (off_t)ASC_OFFT(hd->size, sizeof(hd->size), OCT);
+ if (arcn->sb.st_size == -1)
+ return -1;
arcn->sb.st_mtime = (time_t)(int32_t)asc_u32(hd->mtime,
sizeof(hd->mtime), OCT);
arcn->sb.st_ctime = arcn->sb.st_atime = arcn->sb.st_mtime;
Index: tests/bin/tar/t_tar.sh
===================================================================
RCS file: /pub/NetBSD-CVS/src/tests/bin/tar/t_tar.sh,v
retrieving revision 1.1
diff -u -B -d -u -p -r1.1 t_tar.sh
--- tests/bin/tar/t_tar.sh 17 Mar 2012 16:33:11 -0000 1.1
+++ tests/bin/tar/t_tar.sh 28 Nov 2018 17:23:45 -0000
@@ -45,7 +45,53 @@ append_body() {
atf_check -s eq:0 -o empty -e empty cmp file1.tar file2.tar
}
+atf_test_case rd_base256_size
+rd_base256_size_head() {
+ atf_set "descr" "Test extracting an archive whose member size"
\
+ "is encoded as base-256 number (GNU style)"
+}
+rd_base256_size_body() {
+ # prepare random file data for comparison
+ # take 0x1200CF bytes in order to test that we:
+ # - handle multiple bytes of size field correctly
+ # - do not fail on NUL bytes
+ # - do not fail on char values > 0x80 (with signed char)
+ dd if=/dev/urandom of=reference.bin bs=1179855 count=1
+ # write test archive header
+ # - filename
+ printf 'output.bin' > test.tar
+ # - pad to 100 octets
+ head -c 90 /dev/zero >> test.tar
+ # - mode, uid, gid
+ printf '%07d\0%07d\0%07d\0' 644 177776 177775 >> test.tar
+ # - size (base-256)
+ printf '\x80\0\0\0\0\0\0\0\0\x12\x00\xCF' >> test.tar
+ # - timestamp, checksum
+ printf '%011d\0%06d\0 0' 13377546642 12460 >> test.tar
+ # - pad empty linkname (100 octets)
+ head -c 100 /dev/zero >> test.tar
+ # - magic, user name
+ printf 'ustar \0nobody' >> test.tar
+ # - pad user name field to 32 bytes
+ head -c 26 /dev/zero >> test.tar
+ # - group name
+ printf 'nogroup' >> test.tar
+ # - pad to full block
+ head -c 208 /dev/zero >> test.tar
+ # append file data to the test archive
+ cat reference.bin >> test.tar
+ # pad to full block + append two terminating null blocks
+ head -c 1450 /dev/zero >> test.tar
+
+ # test extracting the test archive
+ atf_check -s eq:0 -o empty -e empty tar -xf test.tar
+
+ # ensure that output.bin is equal to reference.bin
+ atf_check -s eq:0 -o empty -e empty cmp output.bin
reference.bin
+}
+
atf_init_test_cases()
{
atf_add_test_case append
+ atf_add_test_case rd_base256_size
}
--
Best regards,
Michał Górny
signature.asc
Description: This is a digitally signed message part
