From 051cc28c8f586eb3482e5fd9878b2307dbf98e38 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 2 Nov 2017 08:37:06 -0400 Subject: [PATCH] lib: Add STREAM_GETX functions Currently when stream reads fail, for any reason, we assert. While a *great* debugging tool, Asserting on production code is not a good thing. So this is the start of a conversion over to a series of STREAM_GETX functions that do not assert and allow the developer a way to program this gracefully and still clean up. Current code is something like this( taken from redistribute.c because this is dead simple ): afi = stream_getc(client->ibuf); type = stream_getc(client->ibuf); instance = stream_getw(client->ibuf); This code has several issues: 1) There is no failure mode for the stream read other than assert. if afi fails to be read the code stops. 2) stream_getX functions cannot be converted to a failure mode because it is impossible to tell a failure from good data with this api. So this new code will convert to this: STREAM_GETC(client->ibuf, afi); STREAM_GETC(client->ibuf, type); STREAM_GETW(client->ibuf, instance); .... stream_failure: return; We've created a stream_getc2( which does not assert ), but we need a way to allow clean failure mode handling. This is done by macro'ing stream_getX2 functions with the equivalent all uppercase STREAM_GETX functions that include a goto. Signed-off-by: Donald Sharp --- lib/stream.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++---- lib/stream.h | 49 ++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 6 deletions(-) diff --git a/lib/stream.c b/lib/stream.c index f88689f677..0eb790b753 100644 --- a/lib/stream.c +++ b/lib/stream.c @@ -65,12 +65,19 @@ DEFINE_MTYPE_STATIC(LIB, STREAM_FIFO, "Stream FIFO") assert(ENDP_VALID(S, (S)->endp)); \ } while (0) -#define STREAM_BOUND_WARN(S, WHAT) \ - do { \ - zlog_warn("%s: Attempt to %s out of bounds", __func__, \ - (WHAT)); \ - STREAM_WARN_OFFSETS(S); \ - assert(0); \ +#define STREAM_BOUND_WARN(S, WHAT) \ + do { \ + zlog_warn("%s: Attempt to %s out of bounds", __func__, \ + (WHAT)); \ + STREAM_WARN_OFFSETS(S); \ + assert(0); \ + } while (0) + +#define STREAM_BOUND_WARN2(S, WHAT) \ + do { \ + zlog_warn("%s: Attempt to %s out of bounds", __func__, \ + (WHAT)); \ + STREAM_WARN_OFFSETS(S); \ } while (0) /* XXX: Deprecated macro: do not use */ @@ -263,6 +270,21 @@ void stream_forward_endp(struct stream *s, size_t size) } /* Copy from stream to destination. */ +inline bool stream_get2(void *dst, struct stream *s, size_t size) +{ + STREAM_VERIFY_SANE(s); + + if (STREAM_READABLE(s) < size) { + STREAM_BOUND_WARN2(s, "get"); + return false; + } + + memcpy(dst, s->data + s->getp, size); + s->getp += size; + + return true; +} + void stream_get(void *dst, struct stream *s, size_t size) { STREAM_VERIFY_SANE(s); @@ -277,6 +299,19 @@ void stream_get(void *dst, struct stream *s, size_t size) } /* Get next character from the stream. */ +inline bool stream_getc2(struct stream *s, u_char *byte) +{ + STREAM_VERIFY_SANE(s); + + if (STREAM_READABLE(s) < sizeof(u_char)) { + STREAM_BOUND_WARN2(s, "get char"); + return false; + } + *byte = s->data[s->getp++]; + + return true; +} + u_char stream_getc(struct stream *s) { u_char c; @@ -309,6 +344,21 @@ u_char stream_getc_from(struct stream *s, size_t from) return c; } +inline bool stream_getw2(struct stream *s, uint16_t *word) +{ + STREAM_VERIFY_SANE(s); + + if (STREAM_READABLE(s) < sizeof(uint16_t)) { + STREAM_BOUND_WARN2(s, "get "); + return false; + } + + *word = s->data[s->getp++] << 8; + *word |= s->data[s->getp++]; + + return true; +} + /* Get next word from the stream. */ u_int16_t stream_getw(struct stream *s) { @@ -415,6 +465,24 @@ void stream_get_from(void *dst, struct stream *s, size_t from, size_t size) memcpy(dst, s->data + from, size); } +inline bool stream_getl2(struct stream *s, uint32_t *l) +{ + STREAM_VERIFY_SANE(s); + + if (STREAM_READABLE(s) < sizeof(uint32_t)) { + STREAM_BOUND_WARN2(s, "get long"); + return false; + } + + *l = (unsigned int)(s->data[s->getp++]) << 24; + *l |= s->data[s->getp++] << 16; + *l |= s->data[s->getp++] << 8; + *l |= s->data[s->getp++]; + + return true; + +} + u_int32_t stream_getl(struct stream *s) { u_int32_t l; diff --git a/lib/stream.h b/lib/stream.h index 7dcdca69f6..1048180fac 100644 --- a/lib/stream.h +++ b/lib/stream.h @@ -181,14 +181,18 @@ extern int stream_put_prefix(struct stream *, struct prefix *); extern int stream_put_labeled_prefix(struct stream *, struct prefix *, mpls_label_t *); extern void stream_get(void *, struct stream *, size_t); +extern bool stream_get2(void *data, struct stream *s, size_t size); extern void stream_get_from(void *, struct stream *, size_t, size_t); extern u_char stream_getc(struct stream *); +extern bool stream_getc2(struct stream *s, u_char *byte); extern u_char stream_getc_from(struct stream *, size_t); extern u_int16_t stream_getw(struct stream *); +extern bool stream_getw2(struct stream *s, uint16_t *word); extern u_int16_t stream_getw_from(struct stream *, size_t); extern u_int32_t stream_get3(struct stream *); extern u_int32_t stream_get3_from(struct stream *, size_t); extern u_int32_t stream_getl(struct stream *); +extern bool stream_getl2(struct stream *s, uint32_t *l); extern u_int32_t stream_getl_from(struct stream *, size_t); extern uint64_t stream_getq(struct stream *); extern uint64_t stream_getq_from(struct stream *, size_t); @@ -257,4 +261,49 @@ static inline uint8_t *ptr_get_be32(uint8_t *ptr, uint32_t *out) return ptr + 4; } +/* + * so Normal stream_getX functions assert. Which is anathema + * to keeping a daemon up and running when something goes south + * Provide a stream_getX2 functions that do not assert. + * In addition provide these macro's that upon failure + * goto stream_failure. This is modeled upon some NL_XX + * macros in the linux kernel. + * + * This change allows for proper memory freeing + * after we've detected an error. + * + * In the future we will be removing the assert in + * the stream functions but we need a transition + * plan. + */ +#define STREAM_GETC(S, P) \ + do { \ + uint8_t _pval; \ + if (!stream_getc2((S), &_pval)) \ + goto stream_failure; \ + (P) = _pval; \ + } while (0) + +#define STREAM_GETW(S, P) \ + do { \ + uint16_t _pval; \ + if (!stream_getw2((S), &_pval)) \ + goto stream_failure; \ + (P) = _pval; \ + } while (0) + +#define STREAM_GETL(S, P) \ + do { \ + uint32_t _pval; \ + if (!stream_getl2((S), &_pval)) \ + goto stream_failure; \ + (P) = _pval; \ + } while (0) + +#define STREAM_GET(P, STR, SIZE) \ + do { \ + if (!stream_get2((P), (STR), (SIZE))) \ + goto stream_failure; \ + } while (0) + #endif /* _ZEBRA_STREAM_H */ -- 2.39.5