[PATCH] libf2fs: quote non-printables when displaying "disk model"

[PATCH] libf2fs: quote non-printables when displaying "disk model"

[Adam Borowski]

One offender I own has:
STORAGE DEVICE  1404x05xe3x07QGENEx00%x00x00x00x00x00x00x00x00x00x00x00x00x170x04

I limited it to pure ASCII only, but you might want to allow high-bit bytes,
either validating UTF-8 or assuming that mojibake is not as bad as beep and
zeroes as in the above example.  Or, cutting off at the first control
character might be a good alternative too.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 lib/libf2fs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index 5ef0214..8782afc 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -884,8 +884,9 @@ int get_device_info(int i)
 
 			MSG(0, "Info: [%s] Disk Model: ",
 					dev->path);
-			while (reply_buffer[i] != '`' && i < 80)
-				printf("%c", reply_buffer[i++]);
+			for (; reply_buffer[i] != '`' && i < 80; i++)
+				printf((reply_buffer[i] >= ' ' && reply_buffer[i] < 127) ?
+				       "%c" : "x%02x", reply_buffer[i]);
 			printf("\n");
 		}
 #endif
-- 
2.17.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] libf2fs: quote non-printables when displaying "disk model"

[Jaegeuk Kim]

On 05/07, Adam Borowski wrote:
> One offender I own has:
> STORAGE DEVICE  1404x05xe3x07QGENEx00%x00x00x00x00x00x00x00x00x00x00x00x00x170x04
> 
> I limited it to pure ASCII only, but you might want to allow high-bit bytes,
> either validating UTF-8 or assuming that mojibake is not as bad as beep and
> zeroes as in the above example.  Or, cutting off at the first control
> character might be a good alternative too.
> 
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> ---
>  lib/libf2fs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
> index 5ef0214..8782afc 100644
> --- a/lib/libf2fs.c
> +++ b/lib/libf2fs.c
> @@ -884,8 +884,9 @@ int get_device_info(int i)
>  
>  			MSG(0, "Info: [%s] Disk Model: ",
>  					dev->path);
> -			while (reply_buffer[i] != '`' && i < 80)
> -				printf("%c", reply_buffer[i++]);
> +			for (; reply_buffer[i] != '`' && i < 80; i++)
> +				printf((reply_buffer[i] >= ' ' && reply_buffer[i] < 127) ?

How about printing only valid characters here? In my virtualbox, this is
showing garbage characters.

Thanks,

> +				       "%c" : "x%02x", reply_buffer[i]);
>  			printf("\n");
>  		}
>  #endif
> -- 
> 2.17.0

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH v2] libf2fs: read "disk model" from SCSI ioctl the same way kernel does

[Adam Borowski]

On Mon, May 07, 2018 at 02:58:11PM -0700, Jaegeuk Kim wrote:
> On 05/07, Adam Borowski wrote:
> > One offender I own has:
> > STORAGE DEVICE  1404x05xe3x07QGENEx00%x00x00x00x00x00x00x00x00x00x00x00x00x170x04
> > 
> > I limited it to pure ASCII only, but you might want to allow high-bit bytes,
> > either validating UTF-8 or assuming that mojibake is not as bad as beep and
> > zeroes as in the above example.  Or, cutting off at the first control
> > character might be a good alternative too.
> 
> How about printing only valid characters here? In my virtualbox, this is
> showing garbage characters.

I dug more, and it turns out that both the old way and what I had proposed
are totally wrong.  This field is not a real (ie, zero-terminated) string,
and its length is 16 bytes rather than 64 as in current code.  I went
through all SD card readers and USB sticks I had at hand, and indeed the
value is always padded with spaces.  Stuff past byte 16 might be zeroes,
spaces or garbage -- for real (SATA) disks it's clean, for USB stuff it
tends to be junk.  There's a four-byte "rev" field that's sometimes
interesting, but even if we want to print it, it would require a separator.

So, here's another stab:

-- >8 --
Subject: libf2fs: read "disk model" from SCSI ioctl the same way kernel does

Ref: drivers/scsi/scsi_scan.c scsi_add_lun()

Ie, fixed-width 16 bytes, assumed to be filled with spaces -- NOT
null-terminated; comments suggest that in some cases this field can be
truncated and filled with nulls but printf is fine with that.

The old code did read up to 64 characters, which produced garbage for
at least some USB-attached card readers.  It also special-cased '`'
character which the kernel does not.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 lib/libf2fs.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index 5ef0214..38480c5 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -880,13 +880,8 @@ int get_device_info(int i)
 		io_hdr.timeout = 1000;
 
 		if (!ioctl(fd, SG_IO, &io_hdr)) {
-			int i = 16;
-
-			MSG(0, "Info: [%s] Disk Model: ",
-					dev->path);
-			while (reply_buffer[i] != '`' && i < 80)
-				printf("%c", reply_buffer[i++]);
-			printf("\n");
+			MSG(0, "Info: [%s] Disk Model: %.16s\n",
+					dev->path, reply_buffer+16);
 		}
 #endif
 	} else {
-- 
2.17.0

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH v2] libf2fs: read "disk model" from SCSI ioctl the same way kernel does

[Chao Yu]

On 2018/5/25 5:17, Adam Borowski wrote:
> On Mon, May 07, 2018 at 02:58:11PM -0700, Jaegeuk Kim wrote:
>> On 05/07, Adam Borowski wrote:
>>> One offender I own has:
>>> STORAGE DEVICE  1404x05xe3x07QGENEx00%x00x00x00x00x00x00x00x00x00x00x00x00x170x04
>>>
>>> I limited it to pure ASCII only, but you might want to allow high-bit bytes,
>>> either validating UTF-8 or assuming that mojibake is not as bad as beep and
>>> zeroes as in the above example.  Or, cutting off at the first control
>>> character might be a good alternative too.
>>
>> How about printing only valid characters here? In my virtualbox, this is
>> showing garbage characters.
> 
> I dug more, and it turns out that both the old way and what I had proposed
> are totally wrong.  This field is not a real (ie, zero-terminated) string,
> and its length is 16 bytes rather than 64 as in current code.  I went
> through all SD card readers and USB sticks I had at hand, and indeed the
> value is always padded with spaces.  Stuff past byte 16 might be zeroes,
> spaces or garbage -- for real (SATA) disks it's clean, for USB stuff it
> tends to be junk.  There's a four-byte "rev" field that's sometimes
> interesting, but even if we want to print it, it would require a separator.
> 
> So, here's another stab:
> 
> -- >8 --
> Subject: libf2fs: read "disk model" from SCSI ioctl the same way kernel does
> 
> Ref: drivers/scsi/scsi_scan.c scsi_add_lun()
> 
> Ie, fixed-width 16 bytes, assumed to be filled with spaces -- NOT
> null-terminated; comments suggest that in some cases this field can be
> truncated and filled with nulls but printf is fine with that.
> 
> The old code did read up to 64 characters, which produced garbage for
> at least some USB-attached card readers.  It also special-cased '`'
> character which the kernel does not.
> 
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot