Re: Segfault fix in scsi_id-0.6

Re: Segfault fix in scsi_id-0.6

[Patrick Mansfield]

[following up on hotplug-devel ...]

On Tue, Sep 28, 2004 at 03:33:59PM -0600, Andrew Patterson wrote:
> Hi Patrick,
> 
> I found a segfault when running scsi_id on an IA-64 box.  I assume you
> will get the same behavior on any box where an int is 32 bits and not
> 64.  I have included a patch to fix the problem.  Note the "- (char *)0
> change that I stole from lmdd.
> 
> Andrew
> 
> 
> 
> diff -Nur scsi_id-0.6/scsi_id.c scsi_id-0.6.new/scsi_id.c
> --- scsi_id-0.6/scsi_id.c       2004-07-30 14:33:59.000000000 -0600
> +++ scsi_id-0.6.new/scsi_id.c   2004-09-28 15:24:44.000000000 -0600
> @@ -713,7 +713,7 @@
> 
>  #define ALIGN   512
>         unaligned_buf = malloc(MAX_SERIAL_LEN + ALIGN);
> -       serial = (char*) (((int) unaligned_buf + (ALIGN - 1)) & ~(ALIGN
> - 1));
> +       serial = (char*) (((unaligned_buf - (char *)0) + (ALIGN - 1)) &
> ~(ALIGN - 1));
>         dprintf("buffer unaligned 0x%p; aligned 0x%p\n", unaligned_buf,
> serial); #undef ALIGN

Thanks for pointing this out, patch works fine.

But how about just casting to unsigned long instead? Like:

Index: scsi_id.c
=================================RCS file: /home/patman/CVS/scsi_id/scsi_id/scsi_id.c,v
retrieving revision 1.36
diff -u -r1.36 scsi_id.c
--- scsi_id.c	30 Jul 2004 20:33:59 -0000	1.36
+++ scsi_id.c	29 Sep 2004 00:38:59 -0000
@@ -713,7 +713,8 @@
 
 #define ALIGN   512
 	unaligned_buf = malloc(MAX_SERIAL_LEN + ALIGN);
-	serial = (char*) (((int) unaligned_buf + (ALIGN - 1)) & ~(ALIGN - 1));
+	serial = (char*) (((unsigned long) unaligned_buf + (ALIGN - 1))
+			  & ~(ALIGN - 1));
 	dprintf("buffer unaligned 0x%p; aligned 0x%p\n", unaligned_buf, serial);
 #undef ALIGN
 
-- Patrick Mansfield


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: Segfault fix in scsi_id-0.6

[Dave Dodge]

On Tue, Sep 28, 2004 at 05:45:39PM -0700, Patrick Mansfield wrote:
> On Tue, Sep 28, 2004 at 03:33:59PM -0600, Andrew Patterson wrote:

> > +       serial = (char*) (((unaligned_buf - (char *)0) + (ALIGN - 1)) &
> > ~(ALIGN - 1));

Quick comment: the C compiler will treat the (char*)0 as a null
pointer constant.  C makes no guarantee that the resulting value will
have anything to do with address 0.  I believe there are architectures
where a null pointer has all-bits-on or some other non-zero
representation, though I don't know if Linux has ever been made to run
on any of them.

> But how about just casting to unsigned long instead? Like:

> +	serial = (char*) (((unsigned long) unaligned_buf + (ALIGN - 1))
> +			  & ~(ALIGN - 1));

None of this pointer manipulation is technically defined to work, but
from a practical standpoint this will probably be safe.  There's still
the possibility that an unsigned long can't hold all pointer values.
If you're willing to rely on a C99 construct, you could include
<stdint.h> and use uintptr_t instead.

                                                  -Dave Dodge


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: Segfault fix in scsi_id-0.6

[Andrew Patterson]

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

On Wed, 2004-09-29 at 15:03 -0400, Dave Dodge wrote:
> On Tue, Sep 28, 2004 at 05:45:39PM -0700, Patrick Mansfield wrote:
> > On Tue, Sep 28, 2004 at 03:33:59PM -0600, Andrew Patterson wrote:
> 
> > > +       serial = (char*) (((unaligned_buf - (char *)0) + (ALIGN - 1)) &
> > > ~(ALIGN - 1));
> 
> Quick comment: the C compiler will treat the (char*)0 as a null
> pointer constant.  C makes no guarantee that the resulting value will
> have anything to do with address 0.  I believe there are architectures
> where a null pointer has all-bits-on or some other non-zero
> representation, though I don't know if Linux has ever been made to run
> on any of them.
> 
> > But how about just casting to unsigned long instead? Like:
> 
> > +	serial = (char*) (((unsigned long) unaligned_buf + (ALIGN - 1))
> > +			  & ~(ALIGN - 1));
> 

This works on i386 and ia64.  As Dave points out, it may not work on
architectures where a pointer size != unsigned long.

Andrew Patterson

> None of this pointer manipulation is technically defined to work, but
> from a practical standpoint this will probably be safe.  There's still
> the possibility that an unsigned long can't hold all pointer values.
> If you're willing to rely on a C99 construct, you could include
> <stdint.h> and use uintptr_t instead.
> 
>                                                   -Dave Dodge
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]