| Message ID | 1374761521-17603-1-git-send-email-guaneryu@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
> 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 -- 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
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 -- 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
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 -- 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 --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);
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(-)