[PATCH] SCSI: sanitize INQUIRY strings

[PATCH] SCSI: sanitize INQUIRY strings

[Alan Stern]

This patch (as766) sanitizes the Vendor, Product, and Revision strings
contained in an INQUIRY result, by setting all non-graphic or
non-ASCII characters to ' '.  Since the standard disallows such
characters, this will affect only non-compliant devices.

The most prominent effect will be to prevent stray NUL characters from
terminating one of these strings early (which can prevent a blacklist
match).

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

There is a small possibility that this may cause a problem for some users.  
But nobody on the mailing raised any serious objections, so I'm submitting 
it.  I know of one person it will definitely help.

Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -148,27 +148,19 @@ static void scsi_unlock_floptical(struct
 static void print_inquiry(unsigned char *inq_result)
 {
 	int i;
+	int n = inq_result[4] + 5;
 
 	printk(KERN_NOTICE "  Vendor: ");
 	for (i = 8; i < 16; i++)
-		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
-		else
-			printk(" ");
+		printk("%c", (i < n ? inq_result[i] : ' '));
 
 	printk("  Model: ");
 	for (i = 16; i < 32; i++)
-		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
-		else
-			printk(" ");
+		printk("%c", (i < n ? inq_result[i] : ' '));
 
 	printk("  Rev: ");
 	for (i = 32; i < 36; i++)
-		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
-			printk("%c", inq_result[i]);
-		else
-			printk(" ");
+		printk("%c", (i < n ? inq_result[i] : ' '));
 
 	printk("\n");
 
@@ -463,13 +455,14 @@ void scsi_target_reap(struct scsi_target
  *     INQUIRY data is in @inq_result; the scsi_level and INQUIRY length
  *     are copied to the scsi_device any flags value is stored in *@bflags.
  **/
-static int scsi_probe_lun(struct scsi_device *sdev, char *inq_result,
+static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 			  int result_len, int *bflags)
 {
 	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
 	int first_inquiry_len, try_inquiry_len, next_inquiry_len;
 	int response_len = 0;
 	int pass, count, result;
+	int i;
 	struct scsi_sense_hdr sshdr;
 
 	*bflags = 0;
@@ -526,6 +519,12 @@ static int scsi_probe_lun(struct scsi_de
 		if (response_len > 255)
 			response_len = first_inquiry_len;	/* sanity */
 
+		/* Sanitize the Vendor, Product, and Revision fields. */
+		for (i = 8; i < 36; ++i) {
+			if (inq_result[i] < 0x20 || inq_result[i] > 0x7e)
+				inq_result[i] = ' ';
+		}
+
 		/*
 		 * Get any flags for this device.
 		 *
@@ -628,7 +627,8 @@ static int scsi_probe_lun(struct scsi_de
  *     SCSI_SCAN_NO_RESPONSE: could not allocate or setup a scsi_device
  *     SCSI_SCAN_LUN_PRESENT: a new scsi_device was allocated and initialized
  **/
-static int scsi_add_lun(struct scsi_device *sdev, char *inq_result, int *bflags)
+static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
+		int *bflags)
 {
 	/*
 	 * XXX do not save the inquiry, since it can change underneath us,

Re: [PATCH] SCSI: sanitize INQUIRY strings

[Matthew Wilcox]

On Mon, Aug 21, 2006 at 12:03:21PM -0400, Alan Stern wrote:
> This patch (as766) sanitizes the Vendor, Product, and Revision strings
> contained in an INQUIRY result, by setting all non-graphic or
> non-ASCII characters to ' '.  Since the standard disallows such
> characters, this will affect only non-compliant devices.

I thiink you attached the wrong patch; it doesn't match the description
at all.  Besides, print_inquiry is gone in scsi-misc.

> The most prominent effect will be to prevent stray NUL characters from
> terminating one of these strings early (which can prevent a blacklist
> match).
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> There is a small possibility that this may cause a problem for some users.  
> But nobody on the mailing raised any serious objections, so I'm submitting 
> it.  I know of one person it will definitely help.
> 
> Index: usb-2.6/drivers/scsi/scsi_scan.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/scsi_scan.c
> +++ usb-2.6/drivers/scsi/scsi_scan.c
> @@ -148,27 +148,19 @@ static void scsi_unlock_floptical(struct
>  static void print_inquiry(unsigned char *inq_result)
>  {
>  	int i;
> +	int n = inq_result[4] + 5;
>  
>  	printk(KERN_NOTICE "  Vendor: ");
>  	for (i = 8; i < 16; i++)
> -		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
> -			printk("%c", inq_result[i]);
> -		else
> -			printk(" ");
> +		printk("%c", (i < n ? inq_result[i] : ' '));
>  
>  	printk("  Model: ");
>  	for (i = 16; i < 32; i++)
> -		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
> -			printk("%c", inq_result[i]);
> -		else
> -			printk(" ");
> +		printk("%c", (i < n ? inq_result[i] : ' '));
>  
>  	printk("  Rev: ");
>  	for (i = 32; i < 36; i++)
> -		if (inq_result[i] >= 0x20 && i < inq_result[4] + 5)
> -			printk("%c", inq_result[i]);
> -		else
> -			printk(" ");
> +		printk("%c", (i < n ? inq_result[i] : ' '));
>  
>  	printk("\n");
>  
> @@ -463,13 +455,14 @@ void scsi_target_reap(struct scsi_target
>   *     INQUIRY data is in @inq_result; the scsi_level and INQUIRY length
>   *     are copied to the scsi_device any flags value is stored in *@bflags.
>   **/
> -static int scsi_probe_lun(struct scsi_device *sdev, char *inq_result,
> +static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  			  int result_len, int *bflags)
>  {
>  	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
>  	int first_inquiry_len, try_inquiry_len, next_inquiry_len;
>  	int response_len = 0;
>  	int pass, count, result;
> +	int i;
>  	struct scsi_sense_hdr sshdr;
>  
>  	*bflags = 0;
> @@ -526,6 +519,12 @@ static int scsi_probe_lun(struct scsi_de
>  		if (response_len > 255)
>  			response_len = first_inquiry_len;	/* sanity */
>  
> +		/* Sanitize the Vendor, Product, and Revision fields. */
> +		for (i = 8; i < 36; ++i) {
> +			if (inq_result[i] < 0x20 || inq_result[i] > 0x7e)
> +				inq_result[i] = ' ';
> +		}
> +
>  		/*
>  		 * Get any flags for this device.
>  		 *
> @@ -628,7 +627,8 @@ static int scsi_probe_lun(struct scsi_de
>   *     SCSI_SCAN_NO_RESPONSE: could not allocate or setup a scsi_device
>   *     SCSI_SCAN_LUN_PRESENT: a new scsi_device was allocated and initialized
>   **/
> -static int scsi_add_lun(struct scsi_device *sdev, char *inq_result, int *bflags)
> +static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
> +		int *bflags)
>  {
>  	/*
>  	 * XXX do not save the inquiry, since it can change underneath us,
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SCSI: sanitize INQUIRY strings

[Alan Stern]

On Mon, 21 Aug 2006, Matthew Wilcox wrote:

> On Mon, Aug 21, 2006 at 12:03:21PM -0400, Alan Stern wrote:
> > This patch (as766) sanitizes the Vendor, Product, and Revision strings
> > contained in an INQUIRY result, by setting all non-graphic or
> > non-ASCII characters to ' '.  Since the standard disallows such
> > characters, this will affect only non-compliant devices.
> 
> I thiink you attached the wrong patch; it doesn't match the description
> at all.

No; it does match the description.  But you have to read it carefully, 
because the majority of the patch implements optimizations made possible 
by the small piece that does the actual sanitizing.

>  Besides, print_inquiry is gone in scsi-misc.

The patch was based on 2.6.18-rc4.  Is there a patch file I can download 
with the differences between that kernel and the current SCSI development 
tree?

Alan Stern

Re: [PATCH] SCSI: sanitize INQUIRY strings

[Matthew Wilcox]

On Mon, Aug 21, 2006 at 12:52:18PM -0400, Alan Stern wrote:
> On Mon, 21 Aug 2006, Matthew Wilcox wrote:
> 
> > On Mon, Aug 21, 2006 at 12:03:21PM -0400, Alan Stern wrote:
> > > This patch (as766) sanitizes the Vendor, Product, and Revision strings
> > > contained in an INQUIRY result, by setting all non-graphic or
> > > non-ASCII characters to ' '.  Since the standard disallows such
> > > characters, this will affect only non-compliant devices.
> > 
> > I thiink you attached the wrong patch; it doesn't match the description
> > at all.
> 
> No; it does match the description.  But you have to read it carefully, 
> because the majority of the patch implements optimizations made possible 
> by the small piece that does the actual sanitizing.

The description should have said that.  I don't know what you're trying
to accomplish with pulling the length byte out of the inquiry data, for
example.

In any case, you didn't address my point that a NUL byte is probably
meant to terminate the string, not merely be treated equivalently to
a space.  Of course, we can do anything here, the device is out of spec,
but in terms of trying to ascertain the intention of the drug-crazed
hobo^W^W^W^W engineer who wrote the firmware, I think your interpretation
is less likely.

Re: [PATCH] SCSI: sanitize INQUIRY strings

[Philip R. Auld]

Hi,


Rumor has it that on Mon, Aug 21, 2006 at 11:35:46AM -0600 Matthew Wilcox said:
> On Mon, Aug 21, 2006 at 12:52:18PM -0400, Alan Stern wrote:
> > On Mon, 21 Aug 2006, Matthew Wilcox wrote:
> > 
> > > On Mon, Aug 21, 2006 at 12:03:21PM -0400, Alan Stern wrote:
> > > > This patch (as766) sanitizes the Vendor, Product, and Revision strings
> > > > contained in an INQUIRY result, by setting all non-graphic or
> > > > non-ASCII characters to ' '.  Since the standard disallows such
> > > > characters, this will affect only non-compliant devices.
> > > 
> > > I thiink you attached the wrong patch; it doesn't match the description
> > > at all.
> > 
> > No; it does match the description.  But you have to read it carefully, 
> > because the majority of the patch implements optimizations made possible 
> > by the small piece that does the actual sanitizing.
> 
> The description should have said that.  I don't know what you're trying
> to accomplish with pulling the length byte out of the inquiry data, for
> example.
> 
> In any case, you didn't address my point that a NUL byte is probably
> meant to terminate the string, not merely be treated equivalently to
> a space.  Of course, we can do anything here, the device is out of spec,
> but in terms of trying to ascertain the intention of the drug-crazed
> hobo^W^W^W^W engineer who wrote the firmware, I think your interpretation
> is less likely.

As far as I can tell Alan is not trying to "ascertain the intention"
of the firmware engineer, drug-crazed or otherwise. He is making sure
that the array of bytes is printable. You, I think, are trying to
get him to interepret the out-of-spec values. I think that's a
mistake. It's not a string so NUL byte termination does not 
apply. It's an array of what should be printable characters 
of the specified length.


Cheers,


Phil

-- 
Philip R. Auld, Ph.D.  	        	       Egenera, Inc.    
Software Architect                            165 Forest St.
(508) 858-2628                            Marlboro, MA 01752

Re: [PATCH] SCSI: sanitize INQUIRY strings

[Matthew Wilcox]

On Mon, Aug 21, 2006 at 02:11:32PM -0400, Philip R. Auld wrote:
> As far as I can tell Alan is not trying to "ascertain the intention"
> of the firmware engineer, drug-crazed or otherwise. He is making sure
> that the array of bytes is printable. You, I think, are trying to
> get him to interepret the out-of-spec values. I think that's a
> mistake. It's not a string so NUL byte termination does not 
> apply. It's an array of what should be printable characters 
> of the specified length.

The device is out of spec.  The question is how to handle it.  Alan
thinks that a NUL should be treated as a space.  I think that a NUL
indicates the engineer didn't read the spec and intended the string to
stop there, probably padding with garbage.

Let's take the case of a fictional device that has a vendor string

TJD\0Hje9

I say that should be printed as "TJD     ".  You say that should be
printed as "TJD Hje9".

Re: [PATCH] SCSI: sanitize INQUIRY strings

[Philip R. Auld]

Rumor has it that on Mon, Aug 21, 2006 at 12:27:45PM -0600 Matthew Wilcox said:
> On Mon, Aug 21, 2006 at 02:11:32PM -0400, Philip R. Auld wrote:
> > As far as I can tell Alan is not trying to "ascertain the intention"
> > of the firmware engineer, drug-crazed or otherwise. He is making sure
> > that the array of bytes is printable. You, I think, are trying to
> > get him to interepret the out-of-spec values. I think that's a
> > mistake. It's not a string so NUL byte termination does not 
> > apply. It's an array of what should be printable characters 
> > of the specified length.
> 
> The device is out of spec.  The question is how to handle it. 

I think we agree here.

> Alan thinks that a NUL should be treated as a space.  

Alan seems to think, and I agree, that the NUL should be 
treated as all the other non-printing characters. No special case, 
no guessing.

>I think that a NUL
> indicates the engineer didn't read the spec and intended the string to
> stop there, probably padding with garbage.
> 

This is trying to guess what the engineer really meant, though. What
if the engineer meant it to be a token separator? 

> Let's take the case of a fictional device that has a vendor string
> 
> TJD\0Hje9
> 
> I say that should be printed as "TJD     ".  You say that should be
> printed as "TJD Hje9".

Sure, but what if the Hje9 is not garbage. It's still available in the
proposed patch. It's lost if you overwrite it with spaces. Maybe the
TJD Hje9 requires special handling :)

If it turns out to be garbage couldn't the blacklist entry be set to TJD
and only match the first 3 characters? (I haven't looked at how the
comparison is made).

Indeed it may look better treating the NUL as the end, but I'm arguing
that assuming anything about the intent of the engineer who wrote the
out of spec code is probably a mistake.


Cheers,

Phil



-- 
Philip R. Auld, Ph.D.  	        	       Egenera, Inc.    
Software Architect                            165 Forest St.
(508) 858-2628                            Marlboro, MA 01752

Re: [PATCH] SCSI: sanitize INQUIRY strings

[Alan Stern]

On Mon, 21 Aug 2006, Philip R. Auld wrote:

> > Let's take the case of a fictional device that has a vendor string
> > 
> > TJD\0Hje9
> > 
> > I say that should be printed as "TJD     ".  You say that should be
> > printed as "TJD Hje9".
> 
> Sure, but what if the Hje9 is not garbage. It's still available in the
> proposed patch. It's lost if you overwrite it with spaces. Maybe the
> TJD Hje9 requires special handling :)
> 
> If it turns out to be garbage couldn't the blacklist entry be set to TJD
> and only match the first 3 characters? (I haven't looked at how the
> comparison is made).
> 
> Indeed it may look better treating the NUL as the end, but I'm arguing
> that assuming anything about the intent of the engineer who wrote the
> out of spec code is probably a mistake.

In cases like this, I tend to think it's hopeless to guess at what the 
original engineer had in mind.  The best course is to minimize 
user-visible changes, which means interpreting the NUL the same as we have 
all along -- as a terminator.

Alan Stern

Re: [PATCH] SCSI: sanitize INQUIRY strings

[Alan Stern]

This patch (as766b) sanitizes the Vendor, Product, and Revision strings
contained in an INQUIRY result, by setting all non-graphic or non-ASCII
characters to ' '.  Since the standard disallows such characters, this
will affect only non-compliant devices.

To help maintain backward compatibility, NUL characters are treated
specially.  They are taken as string terminators; they and all the
following characters are set to ' '.  If some valid characters get
erased as a result... well, we weren't seeing them before so we haven't
lost anything.

The primary purpose of this change is to allow blacklist entries to
match devices with illegal Vendor or Product strings.

In addition, the patch updates a couple of function prototypes, giving
inq_result its correct type (unsigned char *).

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -397,6 +397,32 @@ void scsi_target_reap(struct scsi_target
 }
 
 /**
+ * sanitize_inquiry_string - remove non-graphical chars from an INQUIRY result string
+ * @s: INQUIRY result string to sanitize
+ * @len: length of the string
+ *
+ * Description:
+ *	The SCSI spec says that INQUIRY vendor, product, and revision
+ *	strings must consist entirely of graphic ASCII characters,
+ *	padded on the right with spaces.  Since not all devices obey
+ *	this rule, we will replace non-graphic or non-ASCII characters
+ *	with spaces.  Exception: a NUL character is interpreted as a
+ *	string terminator, so all the following characters are set to
+ *	spaces.
+ **/
+static void sanitize_inquiry_string(unsigned char *s, int len)
+{
+	int terminated = 0;
+
+	for (; len > 0; (--len, ++s)) {
+		if (*s == 0)
+			terminated = 1;
+		if (terminated || *s < 0x20 || *s > 0x7e)
+			*s = ' ';
+	}
+}
+
+/**
  * scsi_probe_lun - probe a single LUN using a SCSI INQUIRY
  * @sdev:	scsi_device to probe
  * @inq_result:	area to store the INQUIRY result
@@ -410,7 +436,7 @@ void scsi_target_reap(struct scsi_target
  *     INQUIRY data is in @inq_result; the scsi_level and INQUIRY length
  *     are copied to the scsi_device any flags value is stored in *@bflags.
  **/
-static int scsi_probe_lun(struct scsi_device *sdev, char *inq_result,
+static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 			  int result_len, int *bflags)
 {
 	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
@@ -469,7 +495,11 @@ static int scsi_probe_lun(struct scsi_de
 	}
 
 	if (result == 0) {
-		response_len = (unsigned char) inq_result[4] + 5;
+		sanitize_inquiry_string(&inq_result[8], 8);
+		sanitize_inquiry_string(&inq_result[16], 16);
+		sanitize_inquiry_string(&inq_result[32], 4);
+
+		response_len = inq_result[4] + 5;
 		if (response_len > 255)
 			response_len = first_inquiry_len;	/* sanity */
 
@@ -575,7 +605,8 @@ static int scsi_probe_lun(struct scsi_de
  *     SCSI_SCAN_NO_RESPONSE: could not allocate or setup a scsi_device
  *     SCSI_SCAN_LUN_PRESENT: a new scsi_device was allocated and initialized
  **/
-static int scsi_add_lun(struct scsi_device *sdev, char *inq_result, int *bflags)
+static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
+		int *bflags)
 {
 	/*
 	 * XXX do not save the inquiry, since it can change underneath us,

Re: [PATCH] SCSI: sanitize INQUIRY strings

[Alan Stern]

On Mon, 21 Aug 2006, Matthew Wilcox wrote:

> On Mon, Aug 21, 2006 at 12:52:18PM -0400, Alan Stern wrote:
> > On Mon, 21 Aug 2006, Matthew Wilcox wrote:
> > 
> > > On Mon, Aug 21, 2006 at 12:03:21PM -0400, Alan Stern wrote:
> > > > This patch (as766) sanitizes the Vendor, Product, and Revision strings
> > > > contained in an INQUIRY result, by setting all non-graphic or
> > > > non-ASCII characters to ' '.  Since the standard disallows such
> > > > characters, this will affect only non-compliant devices.
> > > 
> > > I thiink you attached the wrong patch; it doesn't match the description
> > > at all.
> > 
> > No; it does match the description.  But you have to read it carefully, 
> > because the majority of the patch implements optimizations made possible 
> > by the small piece that does the actual sanitizing.
> 
> The description should have said that.  I don't know what you're trying
> to accomplish with pulling the length byte out of the inquiry data, for
> example.

I assumed the point of those optimizations was obvious enough not to need 
any comment.

For example, pulling the length byte out removes a bunch of duplicated
array indexes and additions; in three places it lets us change "i <
inq_result[4] + 5" to "i < n".  Whether the object code ends up being any
shorter will depend on the compiler, of course, but it does reduce
redundancy and opportunity for errors in the source code.

Likewise, replacing the "if" statements by conditional expressions (the 
'?:' operator) eliminates three printk subroutine calls.  Although the 
changes are trivial, in the end 12 lines of code are replaced by 4.  Who 
could complain about that?  Especially if the whole subroutine is going to 
vanish anyway.  (It is still present according to the web interface to the 
scsi-misc repository on www.kernel.org, BTW.)

The description should have mentioned the business about changing the
function prototypes so that they correctly declare inq_result as unsigned
char * rather than char *.  If you insist, I will resend the patch with an 
updated description.

> In any case, you didn't address my point that a NUL byte is probably
> meant to terminate the string, not merely be treated equivalently to
> a space.  Of course, we can do anything here, the device is out of spec,
> but in terms of trying to ascertain the intention of the drug-crazed
> hobo^W^W^W^W engineer who wrote the firmware, I think your interpretation
> is less likely.

Your point was already addressed by Philip Auld, who wrote:

  I wouldn't make an exception for NUL. Trying to guess that the device 
  vendor really meant to NUL-terminate does not seem right to me. 
  Plus it will hide out of spec devices more than "spacing" unprintable 
  characters.

Besides, let's assume you're right and the vendor really did mean to 
terminate the string by putting a NUL there.  The bytes following the 
NUL could be anything, but most likely the vendor would set them to more 
NULs or to spaces.  So the patch would change those NULs to spaces, and 
the result is you end up with exactly the string the vendor intended, 
except now it is padded with spaces to the correct length as required by 
the spec.

On the other hand, if some of the characters following the NUL happen to 
be printable then the patch does change the meaning of the string.  That's 
inevitable; when vendors don't follow the rules then sometimes they will 
be misunderstood.  Who's to say whether the pre-patch interpretation is 
any better than the post-patch interpretation?

Alan Stern

Re: [PATCH] SCSI: sanitize INQUIRY strings

[Matthew Wilcox]

On Mon, Aug 21, 2006 at 02:31:18PM -0400, Alan Stern wrote:
> I assumed the point of those optimizations was obvious enough not to need 
> any comment.

Bad assumption.  When you say "This is what the patch does", then I
assume the whole patch does that.  When you say "The patch does X, and
by the way, does some other minor cleanups", then I know the bits which
make no sense are the cleanups and not needed for X.

> (It is still present according to the web interface to the 
> scsi-misc repository on www.kernel.org, BTW.)

It's not ...
http://git.kernel.org/git/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=blob;h=a24d3461fc788b797f492cfff333fa419e9f3bc3;hb=d2afb3ae04e36dbc6e9eb2d8bd54406ff7b6b3bd;f=drivers/scsi/scsi_scan.c

I took it out with:
http://git.kernel.org/git/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=commitdiff;h=4ff36718ede26ee2da73f2dae94d71e2b06845fc

> Besides, let's assume you're right and the vendor really did mean to 
> terminate the string by putting a NUL there.  The bytes following the 
> NUL could be anything, but most likely the vendor would set them to more 
> NULs or to spaces.  So the patch would change those NULs to spaces, and 
> the result is you end up with exactly the string the vendor intended, 
> except now it is padded with spaces to the correct length as required by 
> the spec.

My suggestion produced the same result as yours for these cases.

> On the other hand, if some of the characters following the NUL happen to 
> be printable then the patch does change the meaning of the string.  That's 
> inevitable; when vendors don't follow the rules then sometimes they will 
> be misunderstood.  Who's to say whether the pre-patch interpretation is 
> any better than the post-patch interpretation?

I say so.  I gave an example where my algorithm produces better results
than yours in my response to Philip Auld.  Do you have an example where
yours gives preferable results?

Re: [PATCH] SCSI: sanitize INQUIRY strings

[Alan Stern]

On Mon, 21 Aug 2006, Matthew Wilcox wrote:

> On Mon, Aug 21, 2006 at 02:31:18PM -0400, Alan Stern wrote:
> > I assumed the point of those optimizations was obvious enough not to need 
> > any comment.
> 
> Bad assumption.  When you say "This is what the patch does", then I
> assume the whole patch does that.  When you say "The patch does X, and
> by the way, does some other minor cleanups", then I know the bits which
> make no sense are the cleanups and not needed for X.

Okay, fine.

> > (It is still present according to the web interface to the 
> > scsi-misc repository on www.kernel.org, BTW.)
> 
> It's not ...
> http://git.kernel.org/git/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=blob;h=a24d3461fc788b797f492cfff333fa419e9f3bc3;hb=d2afb3ae04e36dbc6e9eb2d8bd54406ff7b6b3bd;f=drivers/scsi/scsi_scan.c

Arghh...  My browser's bookmark is set to an old tree node (i.e.,
drivers/scsi under scsi-misc-2.6), so whenever I pull it up I see only the
files that were current at the time the bookmark was created!  A subtle
user-interface problem made possible by GIT.

> I took it out with:
> http://git.kernel.org/git/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=commitdiff;h=4ff36718ede26ee2da73f2dae94d71e2b06845fc
> 
> > Besides, let's assume you're right and the vendor really did mean to 
> > terminate the string by putting a NUL there.  The bytes following the 
> > NUL could be anything, but most likely the vendor would set them to more 
> > NULs or to spaces.  So the patch would change those NULs to spaces, and 
> > the result is you end up with exactly the string the vendor intended, 
> > except now it is padded with spaces to the correct length as required by 
> > the spec.
> 
> My suggestion produced the same result as yours for these cases.
> 
> > On the other hand, if some of the characters following the NUL happen to 
> > be printable then the patch does change the meaning of the string.  That's 
> > inevitable; when vendors don't follow the rules then sometimes they will 
> > be misunderstood.  Who's to say whether the pre-patch interpretation is 
> > any better than the post-patch interpretation?
> 
> I say so.  I gave an example where my algorithm produces better results
> than yours in my response to Philip Auld.  Do you have an example where
> yours gives preferable results?

Well, yes.  Suppose the vendor's string was set to 'Abc\0Def\0'.  My 
algorithm would set it to 'Abc Def ' as desired, whereas yours would 
truncate it to 'Abc     '.

Multiplying far-out examples doesn't prove anything, though.  What we need
to do is decide which strategy works better, most of the time.  Up until
now these NUL characters have been interpreted has string terminators, so
it's probably best to keep that interpretation.  For non-NUL non-graphical 
characters, it's best to continue replacing them with spaces.

I will rework the patch (based on the current scsi_scan.c).

Alan Stern