From d31a7cb4fa59c7a2ff39cd091643c18617b601c1 Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Mon, 12 Dec 2022 13:40:05 -0500 Subject: [PATCH] Address theoretical uninitialized variable usage in zstream Coverity has long complained about the checksum being uninitialized if an END record is processed before its BEGIN record. This should not happen, but there was no code to check for it. I had left this unfixed since it was a low priority issue, but then 9f4ede63d23be4f43ba8dd0ca42c6a773a8eaa8d added another instance of this. I am making an effort to "hold the line" to keep new coverity defect reports from going unaddressed, so I find myself forced to fix this much earlier than I had originally planned to address it. The solution is to maintain a counter and a flag. Then use VERIFY statements to verify the following runtime constraints: * Every record either has a corresponding BEGIN record, is a BEGIN record or is the end of stream END record for replication streams. * BEGIN records cannot be nested. i.e. There must be an END record before another BEGIN record may be seen. Failure to meet these constraints will cause the program to exit. This is sufficient to ensure that the checksum is never accessed when uninitialized. Reported-by: Coverity (CID 1524578) Reported-by: Coverity (CID 1524633) Reported-by: Coverity (CID 1527295) Reviewed-by: Brian Behlendorf Reviewed-by: Damian Szuberski Signed-off-by: Richard Yao Closes #14176 --- cmd/zstream/zstream_decompress.c | 17 +++++++++++++++++ cmd/zstream/zstream_recompress.c | 17 +++++++++++++++++ cmd/zstream/zstream_redup.c | 17 +++++++++++++++++ 3 files changed, 51 insertions(+) diff --git a/cmd/zstream/zstream_decompress.c b/cmd/zstream/zstream_decompress.c index 6e0da0852b..726c3be6dc 100644 --- a/cmd/zstream/zstream_decompress.c +++ b/cmd/zstream/zstream_decompress.c @@ -158,6 +158,8 @@ zstream_do_decompress(int argc, char *argv[]) } fletcher_4_init(); + int begin = 0; + boolean_t seen = B_FALSE; while (sfread(drr, sizeof (*drr), stdin) != 0) { struct drr_write *drrw; uint64_t payload_size = 0; @@ -174,6 +176,8 @@ zstream_do_decompress(int argc, char *argv[]) case DRR_BEGIN: { ZIO_SET_CHECKSUM(&stream_cksum, 0, 0, 0, 0); + VERIFY0(begin++); + seen = B_TRUE; int sz = drr->drr_payloadlen; if (sz != 0) { @@ -191,6 +195,13 @@ zstream_do_decompress(int argc, char *argv[]) case DRR_END: { struct drr_end *drre = &drr->drr_u.drr_end; + /* + * We would prefer to just check --begin == 0, but + * replication streams have an end of stream END + * record, so we must avoid tripping it. + */ + VERIFY3B(seen, ==, B_TRUE); + begin--; /* * Use the recalculated checksum, unless this is * the END record of a stream package, which has @@ -204,6 +215,7 @@ zstream_do_decompress(int argc, char *argv[]) case DRR_OBJECT: { struct drr_object *drro = &drr->drr_u.drr_object; + VERIFY3S(begin, ==, 1); if (drro->drr_bonuslen > 0) { payload_size = DRR_OBJECT_PAYLOAD_SIZE(drro); @@ -215,12 +227,14 @@ zstream_do_decompress(int argc, char *argv[]) case DRR_SPILL: { struct drr_spill *drrs = &drr->drr_u.drr_spill; + VERIFY3S(begin, ==, 1); payload_size = DRR_SPILL_PAYLOAD_SIZE(drrs); (void) sfread(buf, payload_size, stdin); break; } case DRR_WRITE_BYREF: + VERIFY3S(begin, ==, 1); fprintf(stderr, "Deduplicated streams are not supported\n"); exit(1); @@ -228,6 +242,7 @@ zstream_do_decompress(int argc, char *argv[]) case DRR_WRITE: { + VERIFY3S(begin, ==, 1); drrw = &thedrr.drr_u.drr_write; payload_size = DRR_WRITE_PAYLOAD_SIZE(drrw); ENTRY *p; @@ -321,6 +336,7 @@ zstream_do_decompress(int argc, char *argv[]) case DRR_WRITE_EMBEDDED: { + VERIFY3S(begin, ==, 1); struct drr_write_embedded *drrwe = &drr->drr_u.drr_write_embedded; payload_size = @@ -332,6 +348,7 @@ zstream_do_decompress(int argc, char *argv[]) case DRR_FREEOBJECTS: case DRR_FREE: case DRR_OBJECT_RANGE: + VERIFY3S(begin, ==, 1); break; default: diff --git a/cmd/zstream/zstream_recompress.c b/cmd/zstream/zstream_recompress.c index b7370587fc..38ef758f8e 100644 --- a/cmd/zstream/zstream_recompress.c +++ b/cmd/zstream/zstream_recompress.c @@ -138,6 +138,8 @@ zstream_do_recompress(int argc, char *argv[]) fletcher_4_init(); zio_init(); zstd_init(); + int begin = 0; + boolean_t seen = B_FALSE; while (sfread(drr, sizeof (*drr), stdin) != 0) { struct drr_write *drrw; uint64_t payload_size = 0; @@ -155,6 +157,8 @@ zstream_do_recompress(int argc, char *argv[]) case DRR_BEGIN: { ZIO_SET_CHECKSUM(&stream_cksum, 0, 0, 0, 0); + VERIFY0(begin++); + seen = B_TRUE; int sz = drr->drr_payloadlen; if (sz != 0) { @@ -172,6 +176,13 @@ zstream_do_recompress(int argc, char *argv[]) case DRR_END: { struct drr_end *drre = &drr->drr_u.drr_end; + /* + * We would prefer to just check --begin == 0, but + * replication streams have an end of stream END + * record, so we must avoid tripping it. + */ + VERIFY3B(seen, ==, B_TRUE); + begin--; /* * Use the recalculated checksum, unless this is * the END record of a stream package, which has @@ -185,6 +196,7 @@ zstream_do_recompress(int argc, char *argv[]) case DRR_OBJECT: { struct drr_object *drro = &drr->drr_u.drr_object; + VERIFY3S(begin, ==, 1); if (drro->drr_bonuslen > 0) { payload_size = DRR_OBJECT_PAYLOAD_SIZE(drro); @@ -196,12 +208,14 @@ zstream_do_recompress(int argc, char *argv[]) case DRR_SPILL: { struct drr_spill *drrs = &drr->drr_u.drr_spill; + VERIFY3S(begin, ==, 1); payload_size = DRR_SPILL_PAYLOAD_SIZE(drrs); (void) sfread(buf, payload_size, stdin); break; } case DRR_WRITE_BYREF: + VERIFY3S(begin, ==, 1); fprintf(stderr, "Deduplicated streams are not supported\n"); exit(1); @@ -209,6 +223,7 @@ zstream_do_recompress(int argc, char *argv[]) case DRR_WRITE: { + VERIFY3S(begin, ==, 1); drrw = &thedrr.drr_u.drr_write; payload_size = DRR_WRITE_PAYLOAD_SIZE(drrw); /* @@ -295,6 +310,7 @@ zstream_do_recompress(int argc, char *argv[]) { struct drr_write_embedded *drrwe = &drr->drr_u.drr_write_embedded; + VERIFY3S(begin, ==, 1); payload_size = P2ROUNDUP((uint64_t)drrwe->drr_psize, 8); (void) sfread(buf, payload_size, stdin); @@ -304,6 +320,7 @@ zstream_do_recompress(int argc, char *argv[]) case DRR_FREEOBJECTS: case DRR_FREE: case DRR_OBJECT_RANGE: + VERIFY3S(begin, ==, 1); break; default: diff --git a/cmd/zstream/zstream_redup.c b/cmd/zstream/zstream_redup.c index 5807fabcec..8b12303c5d 100644 --- a/cmd/zstream/zstream_redup.c +++ b/cmd/zstream/zstream_redup.c @@ -222,6 +222,8 @@ zfs_redup_stream(int infd, int outfd, boolean_t verbose) char *buf = safe_calloc(bufsz); FILE *ofp = fdopen(infd, "r"); long offset = ftell(ofp); + int begin = 0; + boolean_t seen = B_FALSE; while (sfread(drr, sizeof (*drr), ofp) != 0) { num_records++; @@ -240,6 +242,8 @@ zfs_redup_stream(int infd, int outfd, boolean_t verbose) struct drr_begin *drrb = &drr->drr_u.drr_begin; int fflags; ZIO_SET_CHECKSUM(&stream_cksum, 0, 0, 0, 0); + VERIFY0(begin++); + seen = B_TRUE; assert(drrb->drr_magic == DMU_BACKUP_MAGIC); @@ -266,6 +270,13 @@ zfs_redup_stream(int infd, int outfd, boolean_t verbose) case DRR_END: { struct drr_end *drre = &drr->drr_u.drr_end; + /* + * We would prefer to just check --begin == 0, but + * replication streams have an end of stream END + * record, so we must avoid tripping it. + */ + VERIFY3B(seen, ==, B_TRUE); + begin--; /* * Use the recalculated checksum, unless this is * the END record of a stream package, which has @@ -279,6 +290,7 @@ zfs_redup_stream(int infd, int outfd, boolean_t verbose) case DRR_OBJECT: { struct drr_object *drro = &drr->drr_u.drr_object; + VERIFY3S(begin, ==, 1); if (drro->drr_bonuslen > 0) { payload_size = DRR_OBJECT_PAYLOAD_SIZE(drro); @@ -290,6 +302,7 @@ zfs_redup_stream(int infd, int outfd, boolean_t verbose) case DRR_SPILL: { struct drr_spill *drrs = &drr->drr_u.drr_spill; + VERIFY3S(begin, ==, 1); payload_size = DRR_SPILL_PAYLOAD_SIZE(drrs); (void) sfread(buf, payload_size, ofp); break; @@ -299,6 +312,7 @@ zfs_redup_stream(int infd, int outfd, boolean_t verbose) { struct drr_write_byref drrwb = drr->drr_u.drr_write_byref; + VERIFY3S(begin, ==, 1); num_write_byref_records++; @@ -334,6 +348,7 @@ zfs_redup_stream(int infd, int outfd, boolean_t verbose) case DRR_WRITE: { struct drr_write *drrw = &drr->drr_u.drr_write; + VERIFY3S(begin, ==, 1); payload_size = DRR_WRITE_PAYLOAD_SIZE(drrw); (void) sfread(buf, payload_size, ofp); @@ -346,6 +361,7 @@ zfs_redup_stream(int infd, int outfd, boolean_t verbose) { struct drr_write_embedded *drrwe = &drr->drr_u.drr_write_embedded; + VERIFY3S(begin, ==, 1); payload_size = P2ROUNDUP((uint64_t)drrwe->drr_psize, 8); (void) sfread(buf, payload_size, ofp); @@ -355,6 +371,7 @@ zfs_redup_stream(int infd, int outfd, boolean_t verbose) case DRR_FREEOBJECTS: case DRR_FREE: case DRR_OBJECT_RANGE: + VERIFY3S(begin, ==, 1); break; default: