Source and destination overlap in memcpy

Bug #955791 reported by Tom de Vries on 2012-03-15
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
mawk (Ubuntu)
High
Unassigned
Lucid
Undecided
Unassigned
Maverick
Undecided
Unassigned
Natty
Undecided
Unassigned
Oneiric
Undecided
Unassigned
Precise
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.3.4-20100419. We're carrying version 1.3.3-16ubuntu2 in precise, which is affected by the bug. Same patch as is proposed in this SRU was uploaded to Precise March 22nd (Since we're frozen for beta2, it is delayed for archive admin review).

[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.C.*scan' CAT | grep -v ':0'
4
$ rm -f list[0-9]*
$ cat CAT | mawk -f guts.awk
$ grep -c 'abi/mangle33.C.*scan' list* | grep -v ':0'
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-3.6.1-Debian and LibVEX; rerun with -h for copyright info
==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_strmem.c:635)
==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-1.3.3/mawk -f guts.awk
==9007== Memcheck, a memory error detector
==9007== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==9007== Using Valgrind-3.6.1-Debian and LibVEX; rerun with -h for copyright info
==9007== Command: mawk/mawk-1.3.3/mawk -f guts.awk
==9007==
Running target unix
==9007== Source and destination overlap in memcpy(0x545dd90, 0x545e569, 2087)
==9007== at 0x4C2A1C4: memcpy (mc_replace_strmem.c:635)
==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_fin_buffer(fin) ;
   }
   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.

Tom de Vries (tjvries) wrote :
Tom de Vries (tjvries) wrote :
Thomas Dickey (dickey-his) wrote :

Guido Berhoerster asked about this bug in January; it was fixed in upstream mawk
in 20100220.

Tom de Vries (tjvries) wrote :

I couldn't find 20100220. I could find 20100224 in the changelogs, but no matching tarball on ftp://invisible-island.net/mawk.

The bug is reproducible with upstream 1.3.4 (20091220), and not anymore with mawk-1.3.4-20100419.
The source diff between the 2 contains the memcpy -> memmove fix.

Thomas Dickey (dickey-his) wrote :

Ah - to clarify, the changelog shows dates where there was a released tarball.
The 20100220 refers to a label for an interim snapshot which wasn't released
(and it, in turn contains finer-grained checks that aren't labeled for snapshots).
Interim snapshots are useful for checking for regressions, but not generally
useful to people who are using the program.

Tom de Vries (tjvries) wrote :

Thomas,

thanks for the clarification.

I assigned you as reviewer of the branch, I hope you can approve the path.
BTW, this is my first Ubuntu bug (as well as my first mawk bug) to work on, so I'm unfamiliar with the procedures.

Thanks,
- Tom

Bryce Harrington (bryce) on 2012-03-22
description: updated
Bryce Harrington (bryce) on 2012-03-22
description: updated
Bryce Harrington (bryce) on 2012-03-22
Changed in mawk (Ubuntu):
status: New → Triaged
importance: Undecided → High
Bryce Harrington (bryce) wrote :

Hmm, notice that lucid, maverick, natty, oneiric, precise all carry 1.3.3, so presumably this SRU is applicable to all of them.

description: updated
Bryce Harrington (bryce) on 2012-03-23
description: updated
Bryce Harrington (bryce) on 2012-03-23
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
Bryce Harrington (bryce) on 2012-03-23
Changed in mawk (Ubuntu Precise):
status: Triaged → Fix Committed
tags: added: rls-mgr-p-tracking
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package mawk - 1.3.3-16ubuntu3

---------------
mawk (1.3.3-16ubuntu3) precise; urgency=low

  * Replace memcpy by memmove in FINgets.
    (LP: #955791)
 -- Tom de Vries <email address hidden> Sat, 17 Mar 2012 21:04:54 +0100

Changed in mawk (Ubuntu Precise):
status: Fix Committed → Fix Released

Hello Tom, or anyone else affected,

Accepted mawk into natty-proposed. The package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

tags: added: verification-needed
Martin Pitt (pitti) wrote :

Hello Tom, or anyone else affected,

Accepted mawk into maverick-proposed. The package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Changed in mawk (Ubuntu Lucid):
status: Fix Committed → In Progress
Martin Pitt (pitti) wrote :

Note that I have to reject the lucid and maverick uploads, as they use the very same version number as the natty one (1.3.3-15ubuntu2.1). You have to use different versions, such as 1.3.3-15ubuntu2.0.10.04 and 1.3.3-15ubuntu2.0.10.010 (to be smaller than the natty-proposed version number).

Also, I question the need to fix this in maverick. It's one month before EOL and has survived most of its live without this fix.

Changed in mawk (Ubuntu Maverick):
status: Fix Committed → Won't Fix
Changed in mawk (Ubuntu Oneiric):
status: Fix Committed → In Progress
Martin Pitt (pitti) wrote :

Same for the oneiric upload, I suggest ubuntu2.11.10.

Bryce Harrington (bryce) on 2012-03-23
Changed in mawk (Ubuntu Lucid):
status: In Progress → Fix Committed
Changed in mawk (Ubuntu Oneiric):
status: In Progress → Fix Committed
Martin Pitt (pitti) wrote :

Hello Tom, or anyone else affected,

Accepted mawk into oneiric-proposed. The package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Tom de Vries (tjvries) wrote :

> Accepted mawk into oneiric-proposed.
Installed and tested, the test-case from this bug now passes.

Clint Byrum (clint-fewbar) wrote :

Tom, thanks for testing! On what release of Ubuntu did you verify the fix?

Martin Pitt (pitti) on 2012-03-28
tags: added: verification-done-oneiric
Chris Halse Rogers (raof) wrote :

Hello Tom, or anyone else affected,

Accepted mawk into lucid-proposed. The package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Javier López (javier-lopez) wrote :

I've tested successfully the -proposed package in oneiric

---
Ubuntu Bug Squad volunteer triager
http://wiki.ubuntu.com/BugSquad

Martin Pitt (pitti) wrote :

lucid and natty updates still need verification.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package mawk - 1.3.3-15ubuntu2.11.10

---------------
mawk (1.3.3-15ubuntu2.11.10) oneiric-proposed; urgency=low

  * Replace memcpy by memmove in FINgets.
    (LP: #955791)
 -- Tom de Vries <email address hidden> Sat, 17 Mar 2012 21:04:54 +0100

Changed in mawk (Ubuntu Oneiric):
status: Fix Committed → Fix Released
Javier López (javier-lopez) wrote :

The patch isn't needed in Lucid, I was able to run successfully the testcase without the -proposed update

Tom de Vries (tjvries) wrote :

> The patch isn't needed in Lucid, I was able to run successfully the testcase without the -proposed update

Did you try running with valgrind as well?

description: updated

The fix for this bug has been awaiting testing feedback in the -proposed repository for lucid for more than 90 days. Please test this fix and update the bug appropriately with the results. In the event that the fix for this bug is still not verified 15 days from now, the package will be removed from the -proposed repository.

tags: added: removal-candidate
Changed in mawk (Ubuntu Natty):
status: Fix Committed → Won't Fix
Brian Murray (brian-murray) wrote :

The version of mawk in lucid-proposed has been removed as this bug report was not verified in a timely fashion.

Changed in mawk (Ubuntu Lucid):
status: Fix Committed → Triaged
tags: removed: verification-needed
tags: removed: removal-candidate
Rolf Leggewie (r0lf) wrote :

lucid has seen the end of its life and is no longer receiving any updates. Marking the lucid task for this ticket as "Won't Fix".

Changed in mawk (Ubuntu Lucid):
status: Triaged → Won't Fix
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Bug attachments