* [PATCH] btrfs-progs: fix segfault when listing column OTIME on big endian host
@ 2013-07-25 9:32 Eryu Guan
2013-07-25 11:21 ` Miao Xie
0 siblings, 1 reply; 6+ messages in thread
From: Eryu Guan @ 2013-07-25 9:32 UTC (permalink / raw)
To: linux-btrfs; +Cc: Eryu Guan
The second btrfs command segfaults on big endian host(ppc64)
btrfs subvolume snapshot /mnt/btrfs /mnt/btrfs/snap
btrfs subvolume list -s /mnt/btrfs
And ltrace shows
localtime(0x10029c482d0) = 0
strftime( <no return ...>
--- SIGSEGV (Segmentation fault) ---
The corresponding code
btrfs-list.c:
case BTRFS_LIST_OTIME:
if (subv->otime)
strftime(tstr, 256, "%Y-%m-%d %X",
localtime(&subv->otime));
else
strcpy(tstr, "-");
printf("%s", tstr);
break;
localtime() returned NULL then strftime() got SIGSEGV.
The reason is that ri->otime.sec is stored as little endian but
assigned to 't' without conversion.
Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
btrfs-list.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/btrfs-list.c b/btrfs-list.c
index 4fab858..ca1bae8 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -1052,7 +1052,7 @@ static int __list_subvol_search(int fd, struct root_lookup *root_lookup)
flags = btrfs_root_flags(ri);
if(sh.len >
sizeof(struct btrfs_root_item_v0)) {
- t = ri->otime.sec;
+ t = le64_to_cpu(ri->otime.sec);
ogen = btrfs_root_otransid(ri);
memcpy(uuid, ri->uuid, BTRFS_UUID_SIZE);
memcpy(puuid, ri->parent_uuid, BTRFS_UUID_SIZE);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs-progs: fix segfault when listing column OTIME on big endian host
2013-07-25 9:32 [PATCH] btrfs-progs: fix segfault when listing column OTIME on big endian host Eryu Guan
@ 2013-07-25 11:21 ` Miao Xie
2013-07-25 14:12 ` [PATCH v2] " Eryu Guan
0 siblings, 1 reply; 6+ messages in thread
From: Miao Xie @ 2013-07-25 11:21 UTC (permalink / raw)
To: Eryu Guan; +Cc: linux-btrfs
On thu, 25 Jul 2013 17:32:29 +0800, Eryu Guan wrote:
> The second btrfs command segfaults on big endian host(ppc64)
>
> btrfs subvolume snapshot /mnt/btrfs /mnt/btrfs/snap
> btrfs subvolume list -s /mnt/btrfs
>
> And ltrace shows
>
> localtime(0x10029c482d0) = 0
> strftime( <no return ...>
> --- SIGSEGV (Segmentation fault) ---
>
> The corresponding code
>
> btrfs-list.c:
> case BTRFS_LIST_OTIME:
> if (subv->otime)
> strftime(tstr, 256, "%Y-%m-%d %X",
> localtime(&subv->otime));
> else
> strcpy(tstr, "-");
> printf("%s", tstr);
> break;
>
> localtime() returned NULL then strftime() got SIGSEGV.
>
> The reason is that ri->otime.sec is stored as little endian but
> assigned to 't' without conversion.
>
> Signed-off-by: Eryu Guan <guaneryu@gmail.com>
> ---
> btrfs-list.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/btrfs-list.c b/btrfs-list.c
> index 4fab858..ca1bae8 100644
> --- a/btrfs-list.c
> +++ b/btrfs-list.c
> @@ -1052,7 +1052,7 @@ static int __list_subvol_search(int fd, struct root_lookup *root_lookup)
> flags = btrfs_root_flags(ri);
> if(sh.len >
> sizeof(struct btrfs_root_item_v0)) {
> - t = ri->otime.sec;
> + t = le64_to_cpu(ri->otime.sec);
It is better to use btrfs_stack_timespec_sec() instead of raw convert.
Thanks
Miao
> ogen = btrfs_root_otransid(ri);
> memcpy(uuid, ri->uuid, BTRFS_UUID_SIZE);
> memcpy(puuid, ri->parent_uuid, BTRFS_UUID_SIZE);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] btrfs-progs: fix segfault when listing column OTIME on big endian host
2013-07-25 11:21 ` Miao Xie
@ 2013-07-25 14:12 ` Eryu Guan
2013-07-25 21:25 ` Zach Brown
2013-08-01 21:08 ` David Sterba
0 siblings, 2 replies; 6+ messages in thread
From: Eryu Guan @ 2013-07-25 14:12 UTC (permalink / raw)
To: linux-btrfs; +Cc: Eryu Guan
The second btrfs command segfaults on big endian host(ppc64)
btrfs subvolume snapshot /mnt/btrfs /mnt/btrfs/snap
btrfs subvolume list -s /mnt/btrfs
And ltrace shows
localtime(0x10029c482d0) = 0
strftime( <no return ...>
--- SIGSEGV (Segmentation fault) ---
The corresponding code
btrfs-list.c:
case BTRFS_LIST_OTIME:
if (subv->otime)
strftime(tstr, 256, "%Y-%m-%d %X",
localtime(&subv->otime));
else
strcpy(tstr, "-");
printf("%s", tstr);
break;
localtime() returned NULL then strftime() got SIGSEGV.
The reason is that ri->otime.sec is stored as little endian but
assigned to 't' without conversion.
Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
v1->v2:
use btrfs_stack_timespec_sec() instead of raw convert.
Thanks Miao Xie <miaox@cn.fujitsu.com> for pointing this out.
btrfs-list.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/btrfs-list.c b/btrfs-list.c
index 4fab858..9deddd5 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -1052,7 +1052,7 @@ static int __list_subvol_search(int fd, struct root_lookup *root_lookup)
flags = btrfs_root_flags(ri);
if(sh.len >
sizeof(struct btrfs_root_item_v0)) {
- t = ri->otime.sec;
+ t = btrfs_stack_timespec_sec(&ri->otime);
ogen = btrfs_root_otransid(ri);
memcpy(uuid, ri->uuid, BTRFS_UUID_SIZE);
memcpy(puuid, ri->parent_uuid, BTRFS_UUID_SIZE);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] btrfs-progs: fix segfault when listing column OTIME on big endian host
2013-07-25 14:12 ` [PATCH v2] " Eryu Guan
@ 2013-07-25 21:25 ` Zach Brown
2013-07-26 4:34 ` Eryu Guan
2013-08-01 21:08 ` David Sterba
1 sibling, 1 reply; 6+ messages in thread
From: Zach Brown @ 2013-07-25 21:25 UTC (permalink / raw)
To: Eryu Guan; +Cc: linux-btrfs
> btrfs-list.c:
> case BTRFS_LIST_OTIME:
> if (subv->otime)
> strftime(tstr, 256, "%Y-%m-%d %X",
> localtime(&subv->otime));
> else
> strcpy(tstr, "-");
> printf("%s", tstr);
> break;
>
> localtime() returned NULL then strftime() got SIGSEGV.
>
> The reason is that ri->otime.sec is stored as little endian but
> assigned to 't' without conversion.
That's why localtime() returned null, sure, but it doesn't excuse
strftime() being called with a null *tm! Add some error checking around
localtime(). It should warn that otime is nonsense, not crash.
- z
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] btrfs-progs: fix segfault when listing column OTIME on big endian host
2013-07-25 21:25 ` Zach Brown
@ 2013-07-26 4:34 ` Eryu Guan
0 siblings, 0 replies; 6+ messages in thread
From: Eryu Guan @ 2013-07-26 4:34 UTC (permalink / raw)
To: zab; +Cc: linux-btrfs, Eryu Guan
On Fri, Jul 26, 2013 at 5:25 AM, Zach Brown <zab@redhat.com> wrote:
>> btrfs-list.c:
>> case BTRFS_LIST_OTIME:
>> if (subv->otime)
>> strftime(tstr, 256, "%Y-%m-%d %X",
>> localtime(&subv->otime));
>> else
>> strcpy(tstr, "-");
>> printf("%s", tstr);
>> break;
>>
>> localtime() returned NULL then strftime() got SIGSEGV.
>>
>> The reason is that ri->otime.sec is stored as little endian but
>> assigned to 't' without conversion.
>
> That's why localtime() returned null, sure, but it doesn't excuse
> strftime() being called with a null *tm! Add some error checking around
> localtime(). It should warn that otime is nonsense, not crash.
>
Yes, return value of localtime() should be checked. There're other
places call localtime() or localtime_r() without checking the return
value, I think another patch could fix them all and leave this patch
to fix the root cause.
Thanks,
Eryu Guan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] btrfs-progs: fix segfault when listing column OTIME on big endian host
2013-07-25 14:12 ` [PATCH v2] " Eryu Guan
2013-07-25 21:25 ` Zach Brown
@ 2013-08-01 21:08 ` David Sterba
1 sibling, 0 replies; 6+ messages in thread
From: David Sterba @ 2013-08-01 21:08 UTC (permalink / raw)
To: Eryu Guan; +Cc: linux-btrfs
On Thu, Jul 25, 2013 at 10:12:01PM +0800, Eryu Guan wrote:
> The second btrfs command segfaults on big endian host(ppc64)
>
> btrfs subvolume snapshot /mnt/btrfs /mnt/btrfs/snap
> btrfs subvolume list -s /mnt/btrfs
>
> And ltrace shows
>
> localtime(0x10029c482d0) = 0
> strftime( <no return ...>
> --- SIGSEGV (Segmentation fault) ---
>
> The corresponding code
>
> btrfs-list.c:
> case BTRFS_LIST_OTIME:
> if (subv->otime)
> strftime(tstr, 256, "%Y-%m-%d %X",
> localtime(&subv->otime));
> else
> strcpy(tstr, "-");
> printf("%s", tstr);
> break;
>
> localtime() returned NULL then strftime() got SIGSEGV.
>
> The reason is that ri->otime.sec is stored as little endian but
> assigned to 't' without conversion.
I've sent fixes to both otime endianity and localtime crashes some days
ago and will keep them in integration branch. The reentrant variant of
localtime is safer, though the fix to otime access might be enough to
fix it.
http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg25676.html
http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg25677.html
david
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-01 21:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-25 9:32 [PATCH] btrfs-progs: fix segfault when listing column OTIME on big endian host Eryu Guan
2013-07-25 11:21 ` Miao Xie
2013-07-25 14:12 ` [PATCH v2] " Eryu Guan
2013-07-25 21:25 ` Zach Brown
2013-07-26 4:34 ` Eryu Guan
2013-08-01 21:08 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).