diff mbox

Properly size the leafsize field in the mdrestore_struct struct.

Message ID 1402613853-14956-1-git-send-email-abuchbinder@google.com (mailing list archive)
State New, archived
Headers show

Commit Message

Adam Buchbinder June 12, 2014, 10:57 p.m. UTC
It's 32 bits as defined in ctree.h, but the struct had it as 64 bits.

Found using MemorySanitizer.

Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
---
 btrfs-image.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Adam Buchbinder June 13, 2014, 11:58 p.m. UTC | #1
I'd like to follow up on this a bit, because the way I found it was *weird*.

MSan found an uninitialized write. Reproducing the issue through GDB
showed that there's a struct mdrestore_struct type with a member of
type u64 called 'leafsize' which was... half-initialized? Four bytes
were uninitialized, four were initialized. Looking further, it was
being set by a call to a getter function defined in ctree.h, which
pulls a particular member from a superblock struct... which is of type
u32. Aha! So the struct member was missized. Easy enough to fix.
Except that the mdrestore_struct is initialized using calloc, so the
part that wasn't getting written to was
actually initialized, and the part that *was* was showing up as
uninitialized. And actually contained a sane value, reliably. What was
going on?

Turns out that btrfs's use of zlib, which was the uninstrumented
system version, caused anything allocated by uncompress() to show up
as uninitialized. (Trying to use __msan_unpoison did not work, as I
have an iffy install of the
sanitizer in the first place (it's whatever happened to come with
clang-3.3), and MSanDR (the dynamic-instrumentation tool which marks
all uninstrumented libraries as good) is apparently not trivial to
install. I may just build zlib.) But the point is, it would have been
annoying and sad if this had been a false positive through and
through, but I *did* find an actual issue here using MSan, even though
MSan was pointing in the exact wrong place.

Adam Buchbinder
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Satoru Takeuchi June 18, 2014, 6:01 a.m. UTC | #2
Hi Adam,

(2014/06/13 7:57), Adam Buchbinder wrote:
> It's 32 bits as defined in ctree.h, but the struct had it as 64 bits.
> 
> Found using MemorySanitizer.
> 
> Signed-off-by: Adam Buchbinder <abuchbinder@google.com>

It looks good to me.

Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>

Thanks,
Satoru

> ---
>   btrfs-image.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/btrfs-image.c b/btrfs-image.c
> index cf1fe2d..98d765a 100644
> --- a/btrfs-image.c
> +++ b/btrfs-image.c
> @@ -128,7 +128,7 @@ struct mdrestore_struct {
>   	struct rb_root chunk_tree;
>   	struct list_head list;
>   	size_t num_items;
> -	u64 leafsize;
> +	u32 leafsize;
>   	u64 devid;
>   	u8 uuid[BTRFS_UUID_SIZE];
>   	u8 fsid[BTRFS_FSID_SIZE];
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba June 23, 2014, 1:44 p.m. UTC | #3
On Wed, Jun 18, 2014 at 03:01:32PM +0900, Satoru Takeuchi wrote:
> (2014/06/13 7:57), Adam Buchbinder wrote:
> > It's 32 bits as defined in ctree.h, but the struct had it as 64 bits.
> > 
> > Found using MemorySanitizer.
> > 
> > Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
> 
> It looks good to me.
> 
> Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>

Sidned-off? Did you mean Reviewed-by ?
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Satoru Takeuchi June 23, 2014, 11:12 p.m. UTC | #4
(2014/06/23 22:44), David Sterba wrote:
> On Wed, Jun 18, 2014 at 03:01:32PM +0900, Satoru Takeuchi wrote:
>> (2014/06/13 7:57), Adam Buchbinder wrote:
>>> It's 32 bits as defined in ctree.h, but the struct had it as 64 bits.
>>>
>>> Found using MemorySanitizer.
>>>
>>> Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
>>
>> It looks good to me.
>>
>> Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
>
> Sidned-off? Did you mean Reviewed-by ?
>

Oops, sorry.

Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba June 24, 2014, 8:50 a.m. UTC | #5
On Tue, Jun 24, 2014 at 08:12:30AM +0900, Satoru Takeuchi wrote:
> (2014/06/23 22:44), David Sterba wrote:
> >On Wed, Jun 18, 2014 at 03:01:32PM +0900, Satoru Takeuchi wrote:
> >>(2014/06/13 7:57), Adam Buchbinder wrote:
> >>>It's 32 bits as defined in ctree.h, but the struct had it as 64 bits.
> >>>
> >>>Found using MemorySanitizer.
> >>>
> >>>Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
> >>
> >>It looks good to me.
> >>
> >>Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
> >
> >Sidned-off? Did you mean Reviewed-by ?
> 
> Oops, sorry.
> 
> Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>

Thanks, patch updated.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/btrfs-image.c b/btrfs-image.c
index cf1fe2d..98d765a 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -128,7 +128,7 @@  struct mdrestore_struct {
 	struct rb_root chunk_tree;
 	struct list_head list;
 	size_t num_items;
-	u64 leafsize;
+	u32 leafsize;
 	u64 devid;
 	u8 uuid[BTRFS_UUID_SIZE];
 	u8 fsid[BTRFS_FSID_SIZE];