Util-Linux package development
 help / color / mirror / Atom feed
From: Andrew Nayenko <resver@gmail.com>
To: util-linux@vger.kernel.org
Subject: Re: NTFS volume label not found due to endian conversion bug in libblkid
Date: Sat, 04 Dec 2010 14:22:02 +0300	[thread overview]
Message-ID: <4CFA245A.3020900@gmail.com> (raw)
In-Reply-To: <20101203143031.GJ3077@nb.net.home>

>> I've been having a problem with blkid not finding the volume label
>> on NTFS volumes on my big-endian (PowerPC) system. I tracked the
>> issue down to a bug in the endian conversion code for parsing the
>> MFT $Volume attributes in ntfs.c.
> 
>  Fixed. Thanks!

Another similar typo:

diff --git a/shlibs/blkid/src/superblocks/nilfs.c b/shlibs/blkid/src/superblocks/nilfs.c
index bf16918..1f8f3a6 100644
--- a/shlibs/blkid/src/superblocks/nilfs.c
+++ b/shlibs/blkid/src/superblocks/nilfs.c
@@ -84,7 +84,7 @@ static int probe_nilfs2(blkid_probe pr, const struct blkid_idmag *mag)
    if (!sb)
        return -1;

-   bytes = le32_to_cpu(sb->s_bytes);
+   bytes = le16_to_cpu(sb->s_bytes);
    crc = crc32(le32_to_cpu(sb->s_crc_seed), (unsigned char *)sb, sumoff);
    crc = crc32(crc, sum, 4);
    crc = crc32(crc, (unsigned char *)sb + sumoff + 4, bytes - sumoff - 4);
--

I believe there is a better approach to endianness handling. And compiler is our best
helper here. We just need to make it generate an error when we try to use a field with
wrong (or without any) endianness conversion. To achieve this, fields in structures
should be explicitly declared as little or big endian, e.g.:
	struct sb {
		.....
		xle16_t s_bytes;
		.....
	};
The xle16_t type should be a complex one, not an integral:
	typedef struct { uint16_t __leu16; } xle16_t;
Only appropriate conversion functions should know how to convert those types to
integral ones:
	uint16_t xle16_to_cpu(xle16_t v) { return le16_to_cpu(v.__leu16); }
	xle16_t xcpu_to_le16(uint16_t v) { xle16_t t = {cpu_to_le16(v)}; return t; }
That's it. Now compiler will enforce us to use the right conversion anytime we
access a field, i.e. the following statements will fail to compile:
	uint16_t bytes = sb->s_bytes; /* no conversion */
	uint32_t bytes = xle32_to_cpu(sb->s_bytes); /* wrong conversion */
I use this approach in a real life project (FS driver) and it proved to be a good
"bugkeeper".

I had a look at shlibs/blkid/src/superblocks/*.c and I think that most probers can
adopt this approach. But in some cases it can be problematic:
1) sometimes fields endianness can vary (e.g. befs, linux_raid) and we do not know
   it at compile time;
2) some probers (zfs) prefer to convert fields to native endianness first and then
   just use instead of in-place conversion at any access to the field.

-- 
 Andrew Nayenko <resver@gmail.com>

           reply	other threads:[~2010-12-04 11:22 UTC|newest]

Thread overview: expand[flat|nested]  mbox.gz  Atom feed
 [parent not found: <20101203143031.GJ3077@nb.net.home>]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4CFA245A.3020900@gmail.com \
    --to=resver@gmail.com \
    --cc=util-linux@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox