ndr: fix ndr_pull_string_array() off by one alloc

The correct line should have been

       talloc_realloc(ndr->current_mem_ctx, a, const char *, count + 2);

because if the loop does not increment count on exit (it exits via
break), so count is left pointing at the thing that just got put in.
i.e., if there was one item it is at a[0], count is 0, but we also
need the trailing NULL byte at a[1] and the length is 2. Thus + 2, not
+ 1.

This will not affect ordinary (that is, non-malicious) traffic,
because talloc_realloc will not actually realloc unless it is saving a
kilobyte. Since the allocation grows slowly with the exponent ~1.25,
the actual reallocs will start happening at some point between 512 and
1024 items.

In the example we have, there were 666 pointers, and space for 824 was
allocated.

Rather than doing the +2 realloc, it is simpler to leave it off
altogether; in the common case (<512 items) it is a no-op anyway, and
in the best possible case it reduces the temporary array by 20%.

Credit to OSS-Fuzz.

REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24646

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>

1 files changed, 0 insertions, 2 deletions

diff --git a/librpc/ndr/ndr_string.c b/librpc/ndr/ndr_string.c
index bddab9edd51..77efb3e9848 100644
--- a/librpc/ndr/ndr_string.c
+++ b/librpc/ndr/ndr_string.c
@@ -437,8 +437,6 @@ _PUBLIC_ enum ndr_err_code ndr_pull_string_array(struct ndr_pull *ndr, int ndr_f
 				a[count] = s;
 			}
 		}
-		a = talloc_realloc(ndr->current_mem_ctx, a, const char *, count + 1);
-		NDR_ERR_HAVE_NO_MEMORY(a);
 
 		*_a =a;
 		break;