Source and destination overlap in memcpy
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
mawk (Ubuntu) |
Fix Released
|
High
|
Unassigned | ||
Lucid |
Won't Fix
|
Undecided
|
Unassigned | ||
Maverick |
Won't Fix
|
Undecided
|
Unassigned | ||
Natty |
Won't Fix
|
Undecided
|
Unassigned | ||
Oneiric |
Fix Released
|
Undecided
|
Unassigned | ||
Precise |
Fix Released
|
High
|
Unassigned |
Bug Description
[Problem]
mawk loses data due to an improperly constructed memcpy() call.
[Impact]
Data loss in a data processing utility. Frequency of bugs affecting users is not known, but with the given test case occurs 100%. lucid, maverick, natty, oneiric, and precise all carry mawk 1.3.3 and thus are affected.
[Development Fix]
Fixed upstream as of mawk-1.
[Stable Fix]
lp:~tjvries/ubuntu/natty/mawk/fix-for-955791 is a debdiff for natty that applies this 1-liner change.
[Test Case]
<fill me in with detailed *instructions* on how to reproduce the bug. This will be used by people later on to verify the updated package fixes the problem.>
1.
2.
3.
Broken Behavior:
Fixed Behavior:
[Regression Potential]
<fill me in with a discussion of likelihood and potential severity of regressions and how users could get inadvertently affected.
[Original Report]
Source package: mawk
release: 11.10 x86_64
version: 1.3.3-15ubuntu2
on 11.10, I run this series of commands. Both greps are supposed to return 4.
...
$ grep -c 'abi/mangle33.
4
$ rm -f list[0-9]*
$ cat CAT | mawk -f guts.awk
$ grep -c 'abi/mangle33.
3
....
This doesn't reproduce on 10.10, there indeed the last grep returns 4.
Running valgrind on the mawk invocation reveals a problem:
...
$ cat CAT | valgrind mawk -f guts.awk
==8659== Memcheck, a memory error detector
==8659== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==8659== Using Valgrind-
==8659== Command: mawk -f guts.awk
==8659==
Running target unix
==8659== Source and destination overlap in memcpy(0x545dd90, 0x545e569, 2087)
==8659== at 0x4C2A1C4: memcpy (mc_replace_
==8659== by 0x410FC7: ??? (in /usr/bin/mawk)
==8659== by 0x406BA5: ??? (in /usr/bin/mawk)
==8659== by 0x405F99: ??? (in /usr/bin/mawk)
==8659== by 0x50D430C: (below main) (libc-start.c:226)
==8659==
...
And that reproduces on 10.10 as well.
I recompiled the source package at -O0 with -g3, and reran valgrind:
...
$ cat CAT | valgrind mawk/mawk-
==9007== Memcheck, a memory error detector
==9007== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==9007== Using Valgrind-
==9007== Command: mawk/mawk-
==9007==
Running target unix
==9007== Source and destination overlap in memcpy(0x545dd90, 0x545e569, 2087)
==9007== at 0x4C2A1C4: memcpy (mc_replace_
==9007== by 0x41465E: FINgets (fin.c:329)
==9007== by 0x409674: execute (execute.c:1116)
==9007== by 0x406F8A: main (main.c:66)
==9007==
...
That gives me the violating memcpy:
...
if (p == fin->buff)
{
/* current record is too big for the input buffer, grow buffer */
p = enlarge_
}
else
{
/* move a partial line to front of buffer and try again */
unsigned rr ;
p = (char *) memcpy(fin->buff, p, r = strlen(p)) ;
q = p+r ; rr = fin->nbuffs*BUFFSZ - r ;
if ((r = fillbuff(fin->fd, q, rr)) < rr)
fin->flags |= EOF_FLAG ;
}
...
Using the following patch, I abort when the error condition is detected, and now I can reproduce on both 11.10 and 10.10 without valgrind, and easily hit the error in a gdb session.
...
--- fin.c 2012-03-15 09:03:59.674854168 +0100
+++ fin.c.fixed 2012-03-15 09:02:32.910857237 +0100
@@ -76,6 +76,7 @@
#include "field.h"
#include "symtype.h"
#include "scan.h"
+#include <stdlib.h>
#ifndef NO_FCNTL_H
#include <fcntl.h>
@@ -325,8 +326,19 @@
{
/* move a partial line to front of buffer and try again */
unsigned rr ;
-
- p = (char *) memcpy(fin->buff, p, r = strlen(p)) ;
+ char *src, *dst, *srclast, *dstlast;
+ dst = fin->buff;
+ src = p;
+ r = strlen(p);
+ srclast = src + r - 1;
+ dstlast = dst + r - 1;
+ if (!(srclast < dst || dstlast < src))
+ {
+ fprintf (stderr, "overlapping memcpy (%lx, %lx, %u)\n", dst, src, r);
+ fflush (stderr);
+ abort ();
+ }
+ p = (char *)memcpy (fin->buff, p, r) ;
q = p+r ; rr = fin->nbuffs*BUFFSZ - r ;
if ((r = fillbuff(fin->fd, q, rr)) < rr)
...
Replacing the memcpy with a memmove fixes the problem, and I think it's a sufficient fix.
Related branches
- Bryce Harrington: Approve
- Ubuntu branches: Pending requested
-
Diff: 26 lines (+7/-1)2 files modifieddebian/changelog (+6/-0)
fin.c (+1/-1)
description: | updated |
description: | updated |
Changed in mawk (Ubuntu): | |
status: | New → Triaged |
importance: | Undecided → High |
description: | updated |
Changed in mawk (Ubuntu Oneiric): | |
status: | New → Fix Committed |
Changed in mawk (Ubuntu Natty): | |
status: | New → Fix Committed |
Changed in mawk (Ubuntu Maverick): | |
status: | New → Fix Committed |
Changed in mawk (Ubuntu Lucid): | |
status: | New → Fix Committed |
Changed in mawk (Ubuntu Precise): | |
status: | Triaged → Fix Committed |
tags: | added: rls-mgr-p-tracking |
Changed in mawk (Ubuntu Lucid): | |
status: | In Progress → Fix Committed |
Changed in mawk (Ubuntu Oneiric): | |
status: | In Progress → Fix Committed |
tags: | added: verification-done-oneiric |
description: | updated |
Changed in mawk (Ubuntu Natty): | |
status: | Fix Committed → Won't Fix |
Guido Berhoerster asked about this bug in January; it was fixed in upstream mawk
in 20100220.