binutils: objcopy/strip: fix preserving dates in ar archives

Message ID YVJBM+Iv6UIlOmFL@glebfm.cloud.tilaa.com
State Superseded
Headers show
Series
  • binutils: objcopy/strip: fix preserving dates in ar archives
Related show

Commit Message

Gleb Fotengauer-Malinovskiy Sept. 27, 2021, 10:09 p.m.
After commit 985e0264516 copy_archive function began to pass invalid values to
the utimensat(2) function when it tries to preserve timestamps in ar archives:

$ gcc -c -xc /dev/null -o t.o
$ ar rcsU t.a t.o
$ strace --failed-only -e /time strip -p t.a
utimensat(AT_FDCWD, "steICERy/t.o", [{tv_sec=140428413401793, tv_nsec=48} /* 4451969-04-09T19:36:33.000000048+0000 */, {tv_sec=1632773474, tv_nsec=94919810130032}], 0) = -1 EINVAL (Invalid argument)
strip: steICERy/t.o: cannot set time: Invalid argument
+++ exited with 0 +++
$ strace --failed-only -e /time objcopy -p t.a t1.a
utimensat(AT_FDCWD, "stk5ToKP/t.o", [{tv_sec=640, tv_nsec=48} /* 1970-01-01T00:10:40.000000048+0000 */, {tv_sec=0, tv_nsec=223338299408}], 0) = -1 EINVAL (Invalid argument)
objcopy: stk5ToKP/t.o: cannot set time: Invalid argument
+++ exited with 0 +++

This happens because the bfd_stat_arch_elt implementation for ar
archives fills only the st_mtim.tv_sec part of the st_mtim timespec
structure, but leaves the st_mtim.tv_nsec part and the whole st_atim timespec
untouched leaving them uninitialized.
This behavior did not lead to any problems before because only
the st_atim.tv_sec were passed to the utime(2) function uninitialized,
but it was applied to temporary file, whose access time doesn't affect
archive contents later.

Fixes: 985e0264516 ("PR27725, better objcopy -p times")

	PR binutils/28391
	* binutils/objcopy.c (copy_archive): Clear buf using memset.
---
 binutils/objcopy.c | 1 +
 1 file changed, 1 insertion(+)

-- 
glebfm

Comments

Dmitry V. Levin Sept. 27, 2021, 10:57 p.m. | #1
On Tue, Sep 28, 2021 at 01:09:55AM +0300, Gleb Fotengauer-Malinovskiy wrote:
> After commit 985e0264516 copy_archive function began to pass invalid values to

> the utimensat(2) function when it tries to preserve timestamps in ar archives:

> 

> $ gcc -c -xc /dev/null -o t.o

> $ ar rcsU t.a t.o

> $ strace --failed-only -e /time strip -p t.a

> utimensat(AT_FDCWD, "steICERy/t.o", [{tv_sec=140428413401793, tv_nsec=48} /* 4451969-04-09T19:36:33.000000048+0000 */, {tv_sec=1632773474, tv_nsec=94919810130032}], 0) = -1 EINVAL (Invalid argument)

> strip: steICERy/t.o: cannot set time: Invalid argument

> +++ exited with 0 +++

> $ strace --failed-only -e /time objcopy -p t.a t1.a

> utimensat(AT_FDCWD, "stk5ToKP/t.o", [{tv_sec=640, tv_nsec=48} /* 1970-01-01T00:10:40.000000048+0000 */, {tv_sec=0, tv_nsec=223338299408}], 0) = -1 EINVAL (Invalid argument)

> objcopy: stk5ToKP/t.o: cannot set time: Invalid argument

> +++ exited with 0 +++

> 

> This happens because the bfd_stat_arch_elt implementation for ar

> archives fills only the st_mtim.tv_sec part of the st_mtim timespec

> structure, but leaves the st_mtim.tv_nsec part and the whole st_atim timespec

> untouched leaving them uninitialized.

> This behavior did not lead to any problems before because only

> the st_atim.tv_sec were passed to the utime(2) function uninitialized,

> but it was applied to temporary file, whose access time doesn't affect

> archive contents later.

> 

> Fixes: 985e0264516 ("PR27725, better objcopy -p times")


Thanks for the analysis.

> 	PR binutils/28391

> 	* binutils/objcopy.c (copy_archive): Clear buf using memset.

> ---

>  binutils/objcopy.c | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/binutils/objcopy.c b/binutils/objcopy.c

> index a6c2e0dcc26..867aba32cd4 100644

> --- a/binutils/objcopy.c

> +++ b/binutils/objcopy.c

> @@ -3600,6 +3600,7 @@ copy_archive (bfd *ibfd, bfd *obfd, const char *output_target,

>  

>        if (preserve_dates)

>  	{

> +	  memset(buf, &buf, 0, sizeof(buf));

>  	  stat_status = bfd_stat_arch_elt (this_element, &buf);


Did you mean memset(&buf, 0, sizeof(buf)) ?


-- 
ldv
Gleb Fotengauer-Malinovskiy Sept. 27, 2021, 11:31 p.m. | #2
On Tue, Sep 28, 2021 at 01:57:19AM +0300, Dmitry V. Levin wrote:
> On Tue, Sep 28, 2021 at 01:09:55AM +0300, Gleb Fotengauer-Malinovskiy wrote:

> > After commit 985e0264516 copy_archive function began to pass invalid values to

> > the utimensat(2) function when it tries to preserve timestamps in ar archives:

> > 

> > $ gcc -c -xc /dev/null -o t.o

> > $ ar rcsU t.a t.o

> > $ strace --failed-only -e /time strip -p t.a

> > utimensat(AT_FDCWD, "steICERy/t.o", [{tv_sec=140428413401793, tv_nsec=48} /* 4451969-04-09T19:36:33.000000048+0000 */, {tv_sec=1632773474, tv_nsec=94919810130032}], 0) = -1 EINVAL (Invalid argument)

> > strip: steICERy/t.o: cannot set time: Invalid argument

> > +++ exited with 0 +++

> > $ strace --failed-only -e /time objcopy -p t.a t1.a

> > utimensat(AT_FDCWD, "stk5ToKP/t.o", [{tv_sec=640, tv_nsec=48} /* 1970-01-01T00:10:40.000000048+0000 */, {tv_sec=0, tv_nsec=223338299408}], 0) = -1 EINVAL (Invalid argument)

> > objcopy: stk5ToKP/t.o: cannot set time: Invalid argument

> > +++ exited with 0 +++

> > 

> > This happens because the bfd_stat_arch_elt implementation for ar

> > archives fills only the st_mtim.tv_sec part of the st_mtim timespec

> > structure, but leaves the st_mtim.tv_nsec part and the whole st_atim timespec

> > untouched leaving them uninitialized.

> > This behavior did not lead to any problems before because only

> > the st_atim.tv_sec were passed to the utime(2) function uninitialized,

> > but it was applied to temporary file, whose access time doesn't affect

> > archive contents later.

> > 

> > Fixes: 985e0264516 ("PR27725, better objcopy -p times")

> 

> Thanks for the analysis.

> 

> > 	PR binutils/28391

> > 	* binutils/objcopy.c (copy_archive): Clear buf using memset.

> > ---

> >  binutils/objcopy.c | 1 +

> >  1 file changed, 1 insertion(+)

> > 

> > diff --git a/binutils/objcopy.c b/binutils/objcopy.c

> > index a6c2e0dcc26..867aba32cd4 100644

> > --- a/binutils/objcopy.c

> > +++ b/binutils/objcopy.c

> > @@ -3600,6 +3600,7 @@ copy_archive (bfd *ibfd, bfd *obfd, const char *output_target,

> >  

> >        if (preserve_dates)

> >  	{

> > +	  memset(buf, &buf, 0, sizeof(buf));

> >  	  stat_status = bfd_stat_arch_elt (this_element, &buf);

> 

> Did you mean memset(&buf, 0, sizeof(buf)) ?


Oops!  Looks like I posted something else from what I was testing. :(
Thank you for noticing this. :)

-- 
glebfm

Patch

diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index a6c2e0dcc26..867aba32cd4 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -3600,6 +3600,7 @@  copy_archive (bfd *ibfd, bfd *obfd, const char *output_target,
 
       if (preserve_dates)
 	{
+	  memset(buf, &buf, 0, sizeof(buf));
 	  stat_status = bfd_stat_arch_elt (this_element, &buf);
 
 	  if (stat_status != 0)