1
0
mirror of https://github.com/samba-team/samba.git synced 2025-03-24 10:50:22 +03:00

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>
This commit is contained in:
Douglas Bagnall 2020-08-05 11:04:11 +12:00 committed by Douglas Bagnall
parent 889c461c00
commit bf16cd72b2

View File

@ -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;