mdadm-2.2 SEGFAULT: mdadm --assemble --scan

mdadm-2.2 SEGFAULT: mdadm --assemble --scan

[Andre Noll]

sorry if this is already known/fixed: Assemble() is called from mdadm.c with
the "update" argument equal to NULL:

	Assemble(ss, array_list->devname, mdfd, array_list, configfile,
		NULL, readonly, runstop, NULL, verbose-quiet, force);

But in Assemble.c we have

	if (ident->uuid_set && (!update && strcmp(update, "uuid")!= 0) && ...

which yields a segfault in glibc's strcmp().

Andre
-- 
Jesus not only saves, he also frequently makes backups

Re: mdadm-2.2 SEGFAULT: mdadm --assemble --scan

[Andreas Haumer]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi!

Andre Noll schrieb:
> sorry if this is already known/fixed: Assemble() is called from mdadm.c with
> the "update" argument equal to NULL:
> 
> 	Assemble(ss, array_list->devname, mdfd, array_list, configfile,
> 		NULL, readonly, runstop, NULL, verbose-quiet, force);
> 
> But in Assemble.c we have
> 
> 	if (ident->uuid_set && (!update && strcmp(update, "uuid")!= 0) && ...
> 
> which yields a segfault in glibc's strcmp().
> 
I just found the same problem after upgrading to mdadm-2.2
The logic to test for update not being NULL seems to be
reversed.

I created a small patch which seems to cure the problem
(see attached file)

HTH

- - andreas

- --
Andreas Haumer                     | mailto:andreas@xss.co.at
*x Software + Systeme              | http://www.xss.co.at/
Karmarschgasse 51/2/20             | Tel: +43-1-6060114-0
A-1100 Vienna, Austria             | Fax: +43-1-6060114-71
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFDtqD0xJmyeGcXPhERAsdiAJ0Ve787gscq4VOGtT+9Qp3k62iUEgCgs9pH
Ekg0gkLEk+99XXHw+1ezdu8=
=rh66
-----END PGP SIGNATURE-----

[-- Attachment #2: update_uuid.patch --]
[-- Type: text/plain, Size: 697 bytes --]

Index: mdadm/Assemble.c
===================================================================
RCS file: /home/cvs/repository/distribution/Utilities/mdadm/Assemble.c,v
retrieving revision 1.1.1.7
diff -u -r1.1.1.7 Assemble.c
--- mdadm/Assemble.c	5 Dec 2005 05:56:20 -0000	1.1.1.7
+++ mdadm/Assemble.c	31 Dec 2005 15:01:34 -0000
@@ -219,7 +219,7 @@
 		}
 		if (dfd >= 0) close(dfd);
 
-		if (ident->uuid_set && (!update && strcmp(update, "uuid")!= 0) &&
+		if (ident->uuid_set && (update && strcmp(update, "uuid")!= 0) &&
 		    (!super || same_uuid(info.uuid, ident->uuid, tst->ss->swapuuid)==0)) {
 			if ((inargv && verbose >= 0) || verbose > 0)
 				fprintf(stderr, Name ": %s has wrong uuid.\n",

Re: mdadm-2.2 SEGFAULT: mdadm --assemble --scan

[Bill Davidsen]

Andreas Haumer wrote:

>-----BEGIN PGP SIGNED MESSAGE-----
>Hash: SHA1
>
>Hi!
>
>Andre Noll schrieb:
>  
>
>>sorry if this is already known/fixed: Assemble() is called from mdadm.c with
>>the "update" argument equal to NULL:
>>
>>	Assemble(ss, array_list->devname, mdfd, array_list, configfile,
>>		NULL, readonly, runstop, NULL, verbose-quiet, force);
>>
>>But in Assemble.c we have
>>
>>	if (ident->uuid_set && (!update && strcmp(update, "uuid")!= 0) && ...
>>
>>which yields a segfault in glibc's strcmp().
>>
>>    
>>
>I just found the same problem after upgrading to mdadm-2.2
>The logic to test for update not being NULL seems to be
>reversed.
>
>I created a small patch which seems to cure the problem
>(see attached file)
>
>HTH
>
>- - andreas
>
>- --
>Andreas Haumer                     | mailto:andreas@xss.co.at
>*x Software + Systeme              | http://www.xss.co.at/
>Karmarschgasse 51/2/20             | Tel: +43-1-6060114-0
>A-1100 Vienna, Austria             | Fax: +43-1-6060114-71
>-----BEGIN PGP SIGNATURE-----
>Version: GnuPG v1.4.2 (GNU/Linux)
>Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
>
>iD8DBQFDtqD0xJmyeGcXPhERAsdiAJ0Ve787gscq4VOGtT+9Qp3k62iUEgCgs9pH
>Ekg0gkLEk+99XXHw+1ezdu8=
>=rh66
>-----END PGP SIGNATURE-----
>  
>
>------------------------------------------------------------------------
>
>Index: mdadm/Assemble.c
>===================================================================
>RCS file: /home/cvs/repository/distribution/Utilities/mdadm/Assemble.c,v
>retrieving revision 1.1.1.7
>diff -u -r1.1.1.7 Assemble.c
>--- mdadm/Assemble.c	5 Dec 2005 05:56:20 -0000	1.1.1.7
>+++ mdadm/Assemble.c	31 Dec 2005 15:01:34 -0000
>@@ -219,7 +219,7 @@
> 		}
> 		if (dfd >= 0) close(dfd);
> 
>-		if (ident->uuid_set && (!update && strcmp(update, "uuid")!= 0) &&
>+		if (ident->uuid_set && (update && strcmp(update, "uuid")!= 0) &&
> 		    (!super || same_uuid(info.uuid, ident->uuid, tst->ss->swapuuid)==0)) {
> 			if ((inargv && verbose >= 0) || verbose > 0)
> 				fprintf(stderr, Name ": %s has wrong uuid.\n",
>  
>
Is that right now? Because && evaluates to zero or one left to right, 
the parens and the "!=0" are not needed, and I assume they're in for a 
reason (other than to make the code hard to understand). A comment 
before that if would make the intention clear, I originally though the 
"(!update" was intended to be "!(update" which would explain the parens, 
but that seems wrong.

If it actually works as intended with the patch, perhaps a comment and 
cleanup in 2.3?

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc
  Doing interesting things with small computers since 1979

Re: mdadm-2.2 SEGFAULT: mdadm --assemble --scan

[Andreas Haumer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi!

Bill Davidsen schrieb:
> Andreas Haumer wrote:
> 

- ------------------------------------------------------------------------
>>
Index: mdadm/Assemble.c
===================================================================
RCS file: /home/cvs/repository/distribution/Utilities/mdadm/Assemble.c,v
retrieving revision 1.1.1.7
diff -u -r1.1.1.7 Assemble.c
- --- mdadm/Assemble.c    5 Dec 2005 05:56:20 -0000    1.1.1.7
+++ mdadm/Assemble.c    31 Dec 2005 15:01:34 -0000
@@ -219,7 +219,7 @@
        }
        if (dfd >= 0) close(dfd);
>>
- -        if (ident->uuid_set && (!update && strcmp(update, "uuid")!=
0) &&
+        if (ident->uuid_set && (update && strcmp(update, "uuid")!= 0) &&
            (!super || same_uuid(info.uuid, ident->uuid,
tst->ss->swapuuid)==0)) {
            if ((inargv && verbose >= 0) || verbose > 0)
                fprintf(stderr, Name ": %s has wrong uuid.\n",

>>
> Is that right now? Because && evaluates to zero or one left to right,
> the parens and the "!=0" are not needed, and I assume they're in for a
> reason (other than to make the code hard to understand). A comment
> before that if would make the intention clear, I originally though the
> "(!update" was intended to be "!(update" which would explain the parens,
> but that seems wrong.

I made this modification out of the following reasoning:

It does not make sense to check if update is NULL and
then use it in a strcmp(). It only makes sense to check
if update is _not_ NULL and then do the strcmp()

(a similar code fragment can be found in the same sourcefile
several lines below)

This cures the segfault, but I can not really say if the
whole construct is logically correct (you are right, it
looks suspicious...)
That should be answered by Neil ;-)

- - andreas

- --
Andreas Haumer                     | mailto:andreas@xss.co.at
*x Software + Systeme              | http://www.xss.co.at/
Karmarschgasse 51/2/20             | Tel: +43-1-6060114-0
A-1100 Vienna, Austria             | Fax: +43-1-6060114-71
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFDxQ16xJmyeGcXPhERAs2fAJ44Pta06tMd6yI5HqXuRvYkKbWHSACfcPjD
y+b0ddT/ezuqf+rHurm2+Wo=
=SkuC
-----END PGP SIGNATURE-----

Re: mdadm-2.2 SEGFAULT: mdadm --assemble --scan

[Neil Brown]

On Wednesday January 11, andreas@xss.co.at wrote:
> Index: mdadm/Assemble.c
> ===================================================================
> RCS file: /home/cvs/repository/distribution/Utilities/mdadm/Assemble.c,v
> retrieving revision 1.1.1.7
> diff -u -r1.1.1.7 Assemble.c
> - --- mdadm/Assemble.c    5 Dec 2005 05:56:20 -0000    1.1.1.7
> +++ mdadm/Assemble.c    31 Dec 2005 15:01:34 -0000
> @@ -219,7 +219,7 @@
>         }
>         if (dfd >= 0) close(dfd);
> >>
> - -        if (ident->uuid_set && (!update && strcmp(update, "uuid")!=
> 0) &&
> +        if (ident->uuid_set && (update && strcmp(update, "uuid")!= 0) &&
>             (!super || same_uuid(info.uuid, ident->uuid,
> tst->ss->swapuuid)==0)) {
>             if ((inargv && verbose >= 0) || verbose > 0)
>                 fprintf(stderr, Name ": %s has wrong uuid.\n",
> 
> >>
> > Is that right now? Because && evaluates to zero or one left to right,
> > the parens and the "!=0" are not needed, and I assume they're in for a
> > reason (other than to make the code hard to understand). A comment
> > before that if would make the intention clear, I originally though the
> > "(!update" was intended to be "!(update" which would explain the parens,
> > but that seems wrong.
> 
> I made this modification out of the following reasoning:
> 
> It does not make sense to check if update is NULL and
> then use it in a strcmp(). It only makes sense to check
> if update is _not_ NULL and then do the strcmp()
> 
> (a similar code fragment can be found in the same sourcefile
> several lines below)
> 
> This cures the segfault, but I can not really say if the
> whole construct is logically correct (you are right, it
> looks suspicious...)
> That should be answered by Neil ;-)

Sorry it has taken me so long to get back to you on this...

Thanks for reporting the problem.
The correct fix is

diff ./Assemble.c~current~ ./Assemble.c
--- ./Assemble.c~current~	2005-12-05 16:56:20.000000000 +1100
+++ ./Assemble.c	2006-01-24 17:25:20.000000000 +1100
@@ -219,7 +219,7 @@ int Assemble(struct supertype *st, char 
 		}
 		if (dfd >= 0) close(dfd);
 
-		if (ident->uuid_set && (!update && strcmp(update, "uuid")!= 0) &&
+		if (ident->uuid_set && (!update || strcmp(update, "uuid")!= 0) &&
 		    (!super || same_uuid(info.uuid, ident->uuid, tst->ss->swapuuid)==0)) {
 			if ((inargv && verbose >= 0) || verbose > 0)
 				fprintf(stderr, Name ": %s has wrong uuid.\n",


i.e. change the && to and ||.

I guess I'd better do a '2.3'...

Thanks,
NeilBrown