NetBSD Problem Report #44529

From www@NetBSD.org  Mon Feb  7 14:53:54 2011
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 6DD4C63B8BA
	for <gnats-bugs@gnats.NetBSD.org>; Mon,  7 Feb 2011 14:53:54 +0000 (UTC)
Message-Id: <20110207145353.9215B63B842@www.NetBSD.org>
Date: Mon,  7 Feb 2011 14:53:53 +0000 (UTC)
From: Martin.Danielsson@sonyericsson.com
Reply-To: Martin.Danielsson@sonyericsson.com
To: gnats-bugs@NetBSD.org
Subject: [PATCH] fsck_msdos crashes when verifying corrupt file system
X-Send-Pr-Version: www-1.0

>Number:         44529
>Category:       bin
>Synopsis:       [PATCH] fsck_msdos crashes when verifying corrupt file system
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Feb 07 14:55:00 +0000 2011
>Closed-Date:    Sat Feb 12 20:54:57 +0000 2011
>Last-Modified:  Sat Feb 12 20:54:57 +0000 2011
>Originator:     Martin Danielsson
>Release:        N/A (see environment)
>Organization:
Sony Ericsson
>Environment:
The crash was encountered in the Android version of fsck_msdos (Donut, Android 2.0).
>Description:
This bug-report and patch is for an issue that I encountered during the development of the Sony Ericsson X10 Android based phone. Since the version of fsck_msdos used in Android is based on BSD you might be interested in this patch as well.

The problem I encountered was that fsck_msdos crashed when verifying the file system on some specific memory cards.

"When checking file system on a corrupted memory card, fsck_msdos
will sometimes crash. Crash is due to an unexpected sequence number
being read when parsing long file name entries, causing an invalid
pointer being used in overwriting the adjacent _GLOBAL_OFFSET_TABLE_.
Solution is to mark the invalid entry for deletion and prevent use of
the invalid pointer."

The provided patch should apply without problems to the latest version in CVS.
>How-To-Repeat:
The problem is hard to reproduce without applying the specific corruption by hand. When the problem was analyzed I had a memory card given to me by a user that triggered the crash.
>Fix:
--- dir_orig.c	2011-02-07 11:20:51.708888879 +0100
+++ dir.c	2011-02-07 11:28:00.327638983 +0100
@@ -529,7 +529,7 @@
 					vallfn = p;
 					valcl = cl;
 				} else if (shortSum != p[13]
-					   || lidx != (*p & LRNOMASK)) {
+						|| lidx != (*p & LRNOMASK) || !(*p & LRNOMASK)) {
 					if (!invlfn) {
 						invlfn = vallfn;
 						invcl = valcl;
@@ -542,7 +542,8 @@
 				}
 				lidx = *p & LRNOMASK;
 				t = longName + --lidx * 13;
-				for (k = 1; k < 11 && t < longName + sizeof(longName); k += 2) {
+				for (k = 1; k < 11 && t < longName + sizeof(longName)
+						&& t >= longName; k += 2) {
 					if (!p[k] && !p[k + 1])
 						break;
 					*t++ = p[k];
@@ -553,7 +554,8 @@
 						t[-1] = '?';
 				}
 				if (k >= 11)
-					for (k = 14; k < 26 && t < longName + sizeof(longName); k += 2) {
+					for (k = 14; k < 26 && t < longName + sizeof(longName)
+							&& t >= longName; k += 2) {
 						if (!p[k] && !p[k + 1])
 							break;
 						*t++ = p[k];
@@ -561,7 +563,8 @@
 							t[-1] = '?';
 					}
 				if (k >= 26)
-					for (k = 28; k < 32 && t < longName + sizeof(longName); k += 2) {
+					for (k = 28; k < 32 && t < longName + sizeof(longName)
+							&& t >= longName; k += 2) {
 						if (!p[k] && !p[k + 1])
 							break;
 						*t++ = p[k];

>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44529 CVS commit: src/sbin/fsck_msdos
Date: Mon, 7 Feb 2011 12:36:43 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Mon Feb  7 17:36:42 UTC 2011

 Modified Files:
 	src/sbin/fsck_msdos: dir.c

 Log Message:
 PR/44529: Martin Danielsson: fsck_msdos crashes when verifying corrupt file
 system. Avoid using the long name index when it is 0. Refactor the code to
 avoid duplication.


 To generate a diff of this commit:
 cvs rdiff -u -r1.23 -r1.24 src/sbin/fsck_msdos/dir.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: Christos Zoulas <christos@netbsd.org>
Subject: Re: PR/44529 CVS commit: src/sbin/fsck_msdos
Date: Wed, 9 Feb 2011 05:17:44 +0000

 On Mon, Feb 07, 2011 at 05:40:05PM +0000, Christos Zoulas wrote:
  >  Log Message:
  >  PR/44529: Martin Danielsson: fsck_msdos crashes when verifying corrupt file
  >  system. Avoid using the long name index when it is 0. Refactor the code to
  >  avoid duplication.

 I assume you were intending for this for pullup? It applies cleanly to
 both -5 and -4, but I'm somewhat hesitant as it's not all that small,
 I have no easy ability to test it, and I doubt releng does either...

 -- 
 David A. Holland
 dholland@netbsd.org

From: christos@zoulas.com (Christos Zoulas)
To: David Holland <dholland-bugs@netbsd.org>, gnats-bugs@NetBSD.org
Cc: 
Subject: Re: PR/44529 CVS commit: src/sbin/fsck_msdos
Date: Wed, 9 Feb 2011 18:59:39 -0500

 On Feb 9,  5:17am, dholland-bugs@netbsd.org (David Holland) wrote:
 -- Subject: Re: PR/44529 CVS commit: src/sbin/fsck_msdos

 | On Mon, Feb 07, 2011 at 05:40:05PM +0000, Christos Zoulas wrote:
 |  >  Log Message:
 |  >  PR/44529: Martin Danielsson: fsck_msdos crashes when verifying corrupt file
 |  >  system. Avoid using the long name index when it is 0. Refactor the code to
 |  >  avoid duplication.
 | 
 | I assume you were intending for this for pullup? It applies cleanly to
 | both -5 and -4, but I'm somewhat hesitant as it's not all that small,
 | I have no easy ability to test it, and I doubt releng does either...

 The original patch was more complicated and slightly wrong. I am too
 reluctant to pullup because of the lack of testing. I don't think it
 is critical to patch because it only affects direntries that have a zero
 byte in the longname where they are not supposed to.

 christos

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: PR/44529 CVS commit: src/sbin/fsck_msdos
Date: Sat, 12 Feb 2011 20:45:36 +0000

 On Thu, Feb 10, 2011 at 12:00:09AM +0000, Christos Zoulas wrote:
  >  | I assume you were intending for this for pullup? It applies cleanly to
  >  | both -5 and -4, but I'm somewhat hesitant as it's not all that small,
  >  | I have no easy ability to test it, and I doubt releng does either...
  >  
  >  The original patch was more complicated and slightly wrong. I am too
  >  reluctant to pullup because of the lack of testing. I don't think it
  >  is critical to patch because it only affects direntries that have a zero
  >  byte in the longname where they are not supposed to.

 Yeah, me either, so I guess let's just close it.

 -- 
 David A. Holland
 dholland@netbsd.org

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sat, 12 Feb 2011 20:54:57 +0000
State-Changed-Why:
Fixed in HEAD.


>Unformatted:

Home	PR Database Search