From 76daaa23196a5c8c95d18fc8f875fba919a3fed7 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Wed, 4 Jan 2023 11:45:14 -0800 Subject: [PATCH] Incremental step towards fixing nonstandard/nonportable uses of setjmp. As https://en.cppreference.com/w/c/program/setjmp explains, setjmp can only be used in very specific contexts to not trigger UB. PiperOrigin-RevId: 499540912 --- upb/util/compare.c | 23 +++++++++++++++++------ upb/wire/decode.c | 16 ++++++++++------ upb/wire/decode_internal.h | 1 + upb/wire/encode.c | 13 ++++++++----- 4 files changed, 36 insertions(+), 17 deletions(-) diff --git a/upb/util/compare.c b/upb/util/compare.c index 4215411594..56784c264b 100644 --- a/upb/util/compare.c +++ b/upb/util/compare.c @@ -60,9 +60,16 @@ typedef struct { upb_UnknownField* tmp; size_t tmp_size; int depth; + upb_UnknownCompareResult status; jmp_buf err; } upb_UnknownField_Context; +UPB_NORETURN static void upb_UnknownFields_OutOfMemory( + upb_UnknownField_Context* ctx) { + ctx->status = kUpb_UnknownCompareResult_OutOfMemory; + UPB_LONGJMP(ctx->err, 1); +} + static void upb_UnknownFields_Grow(upb_UnknownField_Context* ctx, upb_UnknownField** base, upb_UnknownField** ptr, @@ -72,7 +79,7 @@ static void upb_UnknownFields_Grow(upb_UnknownField_Context* ctx, *base = upb_Arena_Realloc(ctx->arena, *base, old * sizeof(**base), new * sizeof(**base)); - if (!*base) UPB_LONGJMP(ctx->err, kUpb_UnknownCompareResult_OutOfMemory); + if (!*base) upb_UnknownFields_OutOfMemory(ctx); *ptr = *base + old; *end = *base + new; @@ -172,7 +179,8 @@ static upb_UnknownFields* upb_UnknownFields_DoBuild( } case kUpb_WireType_StartGroup: if (--ctx->depth == 0) { - UPB_LONGJMP(ctx->err, kUpb_UnknownCompareResult_MaxDepthExceeded); + ctx->status = kUpb_UnknownCompareResult_MaxDepthExceeded; + UPB_LONGJMP(ctx->err, 1); } field->data.group = upb_UnknownFields_DoBuild(ctx, &ptr); ctx->depth++; @@ -184,7 +192,7 @@ static upb_UnknownFields* upb_UnknownFields_DoBuild( *buf = ptr; upb_UnknownFields* ret = upb_Arena_Malloc(ctx->arena, sizeof(*ret)); - if (!ret) UPB_LONGJMP(ctx->err, kUpb_UnknownCompareResult_OutOfMemory); + if (!ret) upb_UnknownFields_OutOfMemory(ctx); ret->fields = arr_base; ret->size = arr_ptr - arr_base; ret->capacity = arr_end - arr_base; @@ -255,13 +263,13 @@ upb_UnknownCompareResult upb_Message_UnknownFieldsAreEqual(const char* buf1, .depth = max_depth, .tmp = NULL, .tmp_size = 0, + .status = kUpb_UnknownCompareResult_Equal, }; if (!ctx.arena) return kUpb_UnknownCompareResult_OutOfMemory; - int ret = UPB_SETJMP(ctx.err); - - if (UPB_LIKELY(ret == 0)) { + upb_UnknownCompareResult ret; + if (UPB_SETJMP(ctx.err) == 0) { // First build both unknown fields into a sorted data structure (similar // to the UnknownFieldSet in C++). upb_UnknownFields* uf1 = upb_UnknownFields_Build(&ctx, buf1, size1); @@ -273,6 +281,9 @@ upb_UnknownCompareResult upb_Message_UnknownFieldsAreEqual(const char* buf1, } else { ret = kUpb_UnknownCompareResult_NotEqual; } + } else { + ret = ctx.status; + UPB_ASSERT(ret != kUpb_UnknownCompareResult_Equal); } upb_Arena_Free(ctx.arena); diff --git a/upb/wire/decode.c b/upb/wire/decode.c index 6ef1a7aa25..2fe733a504 100644 --- a/upb/wire/decode.c +++ b/upb/wire/decode.c @@ -88,12 +88,14 @@ static const char* _upb_Decoder_DecodeMessage(upb_Decoder* d, const char* ptr, UPB_NORETURN static void* _upb_Decoder_ErrorJmp(upb_Decoder* d, upb_DecodeStatus status) { assert(status != kUpb_DecodeStatus_Ok); - UPB_LONGJMP(d->err, status); + d->status = status; + UPB_LONGJMP(d->err, 1); } const char* _upb_FastDecoder_ErrorJmp(upb_Decoder* d, int status) { assert(status != kUpb_DecodeStatus_Ok); - UPB_LONGJMP(d->err, status); + d->status = status; + UPB_LONGJMP(d->err, 1); return NULL; } @@ -1264,16 +1266,18 @@ upb_DecodeStatus upb_Decode(const char* buf, size_t size, void* msg, state.arena.last_size = arena->last_size; state.arena.cleanup_metadata = arena->cleanup_metadata; state.arena.parent = arena; + state.status = kUpb_DecodeStatus_Ok; - upb_DecodeStatus status = UPB_SETJMP(state.err); - if (UPB_LIKELY(status == kUpb_DecodeStatus_Ok)) { - status = _upb_Decoder_DecodeTop(&state, buf, msg, l); + if (UPB_SETJMP(state.err) == 0) { + state.status = _upb_Decoder_DecodeTop(&state, buf, msg, l); + } else { + UPB_ASSERT(state.status != kUpb_DecodeStatus_Ok); } arena->head.ptr = state.arena.head.ptr; arena->head.end = state.arena.head.end; arena->cleanup_metadata = state.arena.cleanup_metadata; - return status; + return state.status; } #undef OP_FIXPCK_LG2 diff --git a/upb/wire/decode_internal.h b/upb/wire/decode_internal.h index 14cc518c2e..6867736397 100644 --- a/upb/wire/decode_internal.h +++ b/upb/wire/decode_internal.h @@ -54,6 +54,7 @@ typedef struct upb_Decoder { uint16_t options; bool missing_required; upb_Arena arena; + upb_DecodeStatus status; jmp_buf err; #ifndef NDEBUG diff --git a/upb/wire/encode.c b/upb/wire/encode.c index 47565164dd..ab2b8dae20 100644 --- a/upb/wire/encode.c +++ b/upb/wire/encode.c @@ -64,6 +64,7 @@ static uint64_t encode_zz64(int64_t n) { } typedef struct { + upb_EncodeStatus status; jmp_buf err; upb_Arena* arena; char *buf, *ptr, *limit; @@ -81,7 +82,9 @@ static size_t upb_roundup_pow2(size_t bytes) { } UPB_NORETURN static void encode_err(upb_encstate* e, upb_EncodeStatus s) { - UPB_LONGJMP(e->err, s); + UPB_ASSERT(s != kUpb_EncodeStatus_Ok); + e->status = s; + UPB_LONGJMP(e->err, 1); } UPB_NOINLINE @@ -571,6 +574,7 @@ upb_EncodeStatus upb_Encode(const void* msg, const upb_MiniTable* l, upb_encstate e; unsigned depth = (unsigned)options >> 16; + e.status = kUpb_EncodeStatus_Ok; e.arena = arena; e.buf = NULL; e.limit = NULL; @@ -579,13 +583,11 @@ upb_EncodeStatus upb_Encode(const void* msg, const upb_MiniTable* l, e.options = options; _upb_mapsorter_init(&e.sorter); - upb_EncodeStatus status = UPB_SETJMP(e.err); - // Unfortunately we must continue to perform hackery here because there are // code paths which blindly copy the returned pointer without bothering to // check for errors until much later (b/235839510). So we still set *buf to // NULL on error and we still set it to non-NULL on a successful empty result. - if (status == kUpb_EncodeStatus_Ok) { + if (UPB_SETJMP(e.err) == 0) { encode_message(&e, msg, l, size); *size = e.limit - e.ptr; if (*size == 0) { @@ -596,10 +598,11 @@ upb_EncodeStatus upb_Encode(const void* msg, const upb_MiniTable* l, *buf = e.ptr; } } else { + UPB_ASSERT(e.status != kUpb_EncodeStatus_Ok); *buf = NULL; *size = 0; } _upb_mapsorter_destroy(&e.sorter); - return status; + return e.status; }