On 2015/03/17 11:45, Ryan Freeman wrote:
> On Mon, Mar 16, 2015 at 08:00:37PM -0600, Theo de Raadt wrote:
> > If you find the right memcpy, it simply needs to be changed to memmove.
> > Then try to justify it a bit, and pass it to upstream.
Well, usually yes. But then there are special cases like we had in
postgresql which was a different bug.
> Thanks Theo,
>
> After eyeballing the trace again, i noticed it mentioned the exact
> expect function where the failure was, and grep was helpful to
> narrow it down to a single .c file in expect that has that function.
>
> By process of elimination I ended up with ALL the memcpy->memmove
> so now I am going to work backwards to find the exact one that causes
> the abort trap. The good news is it stopped crashing! But not sure
> which one really needed it.
>
> Will work with upstream on getting it integrated, patch is below
> if anyone doesn't mind telling me 'hey that shouldn't be memmove'.
>
> So /before/ i try and narrow it down to just one, is there a chance
> this is a case where they should just all be memmove?
>
>
> $OpenBSD$
> --- exp_inter.c.orig Mon Mar 16 20:27:50 2015
> +++ exp_inter.c Mon Mar 16 20:53:51 2015
> @@ -474,7 +474,7 @@ intRead(
> }
>
> if (cc > 0) {
> - memcpy (esPtr->input.buffer + esPtr->input.use,
> + memmove (esPtr->input.buffer + esPtr->input.use,
> Tcl_GetUnicodeFromObj (esPtr->input.newchars, NULL),
> cc * sizeof (Tcl_UniChar));
> esPtr->input.use += cc;
The above one is most likely OK as memcpy.
All of the remaining ones are overlapping and should be memmove.
> @@ -1564,7 +1564,7 @@ Exp_InteractObjCmd(
> ustring = u->input.buffer;
> if (skip) {
> size -= skip;
> - memcpy(ustring, ustring + skip, size * sizeof(Tcl_UniChar));
> + memmove(ustring, ustring + skip, size * sizeof(Tcl_UniChar));
also: int overflow ahoy? ^^^^^^^^^^^^^^^^^^^^^^^^^^
> }
> }
> u->input.use = size;
> @@ -1824,12 +1824,12 @@ got_action:
> skip += matchLen;
> size -= skip;
> if (size) {
> - memcpy(u->buffer, u->buffer + skip, size);
> + memmove(u->buffer, u->buffer + skip, size);
> }
> } else {
> if (skip) {
> size -= skip;
> - memcpy(u->buffer, u->buffer + skip, size);
> + memmove(u->buffer, u->buffer + skip, size);
> }
> }
> Tcl_SetObjLength(size);
> @@ -2070,12 +2070,12 @@ got_action:
> skip += matchLen;
> size -= skip;
> if (size) {
> - memcpy(u->buffer, u->buffer + skip, size);
> + memmove(u->buffer, u->buffer + skip, size);
> }
> } else {
> if (skip) {
> size -= skip;
> - memcpy(u->buffer, u->buffer + skip, size);
> + memmove(u->buffer, u->buffer + skip, size);
> }
> }
> Tcl_SetObjLength(size);
>
