srcfiles.cxx: Prevent fd and entry leak

Message ID 20241118234356.2712104-1-amerey@redhat.com
State Committed
Headers
Series srcfiles.cxx: Prevent fd and entry leak |

Commit Message

Aaron Merey Nov. 18, 2024, 11:43 p.m. UTC
  Make sure to free fd and the archive entry if an error is encountered
while adding source files to the archive.

Signed-off-by: Aaron Merey <amerey@redhat.com>
---
 src/srcfiles.cxx | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)
  

Comments

Mark Wielaard Nov. 22, 2024, 12:09 a.m. UTC | #1
Hi Aaron,

On Mon, Nov 18, 2024 at 06:43:56PM -0500, Aaron Merey wrote:
> Make sure to free fd and the archive entry if an error is encountered
> while adding source files to the archive.

Looks good to me. But one (paranoid?) nitpick below.

> Signed-off-by: Aaron Merey <amerey@redhat.com>
> ---
>  src/srcfiles.cxx | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/src/srcfiles.cxx b/src/srcfiles.cxx
> index c466b307..b6dd75ad 100644
> --- a/src/srcfiles.cxx
> +++ b/src/srcfiles.cxx
> @@ -312,7 +312,6 @@ void zip_files()
>    struct stat st;
>    char buff[BUFFER_SIZE];
>    int len;
> -  int fd;
>    #ifdef ENABLE_LIBDEBUGINFOD
>    /* Initialize a debuginfod client.  */
>    static unique_ptr <debuginfod_client, void (*)(debuginfod_client*)>
> @@ -325,8 +324,10 @@ void zip_files()
>    int missing_files = 0;
>    for (const auto &pair : debug_sourcefiles)
>    {
> -    fd = -1;
> +    int fd = -1;
>      const std::string &file_path = pair.first;
> +    struct archive_entry *entry = NULL;
> +    string entry_name;

OK, init fd to -1 and entry to NULL.

>      /* Attempt to query debuginfod client to fetch source files.  */
>      #ifdef ENABLE_LIBDEBUGINFOD
> @@ -353,8 +354,8 @@ void zip_files()
>      if (!no_backup)
>      #endif /* ENABLE_LIBDEBUGINFOD */
>        /* Files could not be located using debuginfod, search locally */
> -      if (fd < 0)
> -        fd = open(file_path.c_str(), O_RDONLY);
> +    if (fd < 0)
> +      fd = open(file_path.c_str(), O_RDONLY);

OK, fix indentation (should the comment above also be reindented?)

>      if (fd < 0)
>      {
>        if (verbose)
> @@ -371,11 +372,11 @@ void zip_files()
>        missing_files++;
>        if (verbose)
>          cerr << "Error: Failed to get file status for " << file_path << ": " << strerror(errno) << endl;
> -      continue;
> +      goto next;
>      }
> -    struct archive_entry *entry = archive_entry_new();
> +    entry = archive_entry_new();
>      /* Removing first "/"" to make the path "relative" before zipping, otherwise warnings are raised when unzipping.  */
> -    string entry_name = file_path.substr(file_path.find_first_of('/') + 1);
> +    entry_name = file_path.substr(file_path.find_first_of('/') + 1);
>      archive_entry_set_pathname(entry, entry_name.c_str());
>      archive_entry_copy_stat(entry, &st);
>      if (archive_write_header(a, entry) != ARCHIVE_OK)
> @@ -385,7 +386,7 @@ void zip_files()
>        missing_files++;
>        if (verbose)
>          cerr << "Error: failed to write header for " << file_path << ": " << archive_error_string(a) << endl;
> -      continue;
> +      goto next;
>      }
>  
>      /* Write the file to the zip.  */
> @@ -397,7 +398,7 @@ void zip_files()
>        missing_files++;
>        if (verbose)
>          cerr << "Error: Failed to open file: " << file_path << ": " << strerror(errno) <<endl;
> -      continue;
> +      goto next;
>      }
>      while (len > 0)
>      {

OK, all continues become goto next. Assign locals.

> @@ -409,6 +410,8 @@ void zip_files()
>        }
>        len = read(fd, buff, sizeof(buff));
>      }
> +
> +next:
>      close(fd);
>      archive_entry_free(entry);
>    }

Probably OK.

But technically close (-1) returns an error (which can be ignored).
And libarchive doesn't document what archive_entry_free does with a NULL argument.
To be on the safe side I would just check:

    if (fd >= 0) close (fd);
    if (entry != NULL) archive_entry_free (NULL);

Cheers,

Mark
  
Aaron Merey Nov. 25, 2024, 10:35 p.m. UTC | #2
On Thu, Nov 21, 2024 at 7:09 PM Mark Wielaard <mark@klomp.org> wrote:
>
> Hi Aaron,
>
> On Mon, Nov 18, 2024 at 06:43:56PM -0500, Aaron Merey wrote:
> > Make sure to free fd and the archive entry if an error is encountered
> > while adding source files to the archive.
>
> Looks good to me. But one (paranoid?) nitpick below.
>
> > Signed-off-by: Aaron Merey <amerey@redhat.com>
> > ---
> >  src/srcfiles.cxx | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/srcfiles.cxx b/src/srcfiles.cxx
> > index c466b307..b6dd75ad 100644
> > --- a/src/srcfiles.cxx
> > +++ b/src/srcfiles.cxx
> > @@ -312,7 +312,6 @@ void zip_files()
> >    struct stat st;
> >    char buff[BUFFER_SIZE];
> >    int len;
> > -  int fd;
> >    #ifdef ENABLE_LIBDEBUGINFOD
> >    /* Initialize a debuginfod client.  */
> >    static unique_ptr <debuginfod_client, void (*)(debuginfod_client*)>
> > @@ -325,8 +324,10 @@ void zip_files()
> >    int missing_files = 0;
> >    for (const auto &pair : debug_sourcefiles)
> >    {
> > -    fd = -1;
> > +    int fd = -1;
> >      const std::string &file_path = pair.first;
> > +    struct archive_entry *entry = NULL;
> > +    string entry_name;
>
> OK, init fd to -1 and entry to NULL.
>
> >      /* Attempt to query debuginfod client to fetch source files.  */
> >      #ifdef ENABLE_LIBDEBUGINFOD
> > @@ -353,8 +354,8 @@ void zip_files()
> >      if (!no_backup)
> >      #endif /* ENABLE_LIBDEBUGINFOD */
> >        /* Files could not be located using debuginfod, search locally */
> > -      if (fd < 0)
> > -        fd = open(file_path.c_str(), O_RDONLY);
> > +    if (fd < 0)
> > +      fd = open(file_path.c_str(), O_RDONLY);
>
> OK, fix indentation (should the comment above also be reindented?)
>
> >      if (fd < 0)
> >      {
> >        if (verbose)
> > @@ -371,11 +372,11 @@ void zip_files()
> >        missing_files++;
> >        if (verbose)
> >          cerr << "Error: Failed to get file status for " << file_path << ": " << strerror(errno) << endl;
> > -      continue;
> > +      goto next;
> >      }
> > -    struct archive_entry *entry = archive_entry_new();
> > +    entry = archive_entry_new();
> >      /* Removing first "/"" to make the path "relative" before zipping, otherwise warnings are raised when unzipping.  */
> > -    string entry_name = file_path.substr(file_path.find_first_of('/') + 1);
> > +    entry_name = file_path.substr(file_path.find_first_of('/') + 1);
> >      archive_entry_set_pathname(entry, entry_name.c_str());
> >      archive_entry_copy_stat(entry, &st);
> >      if (archive_write_header(a, entry) != ARCHIVE_OK)
> > @@ -385,7 +386,7 @@ void zip_files()
> >        missing_files++;
> >        if (verbose)
> >          cerr << "Error: failed to write header for " << file_path << ": " << archive_error_string(a) << endl;
> > -      continue;
> > +      goto next;
> >      }
> >
> >      /* Write the file to the zip.  */
> > @@ -397,7 +398,7 @@ void zip_files()
> >        missing_files++;
> >        if (verbose)
> >          cerr << "Error: Failed to open file: " << file_path << ": " << strerror(errno) <<endl;
> > -      continue;
> > +      goto next;
> >      }
> >      while (len > 0)
> >      {
>
> OK, all continues become goto next. Assign locals.
>
> > @@ -409,6 +410,8 @@ void zip_files()
> >        }
> >        len = read(fd, buff, sizeof(buff));
> >      }
> > +
> > +next:
> >      close(fd);
> >      archive_entry_free(entry);
> >    }
>
> Probably OK.
>
> But technically close (-1) returns an error (which can be ignored).
> And libarchive doesn't document what archive_entry_free does with a NULL argument.
> To be on the safe side I would just check:
>
>     if (fd >= 0) close (fd);
>     if (entry != NULL) archive_entry_free (NULL);

Thanks Mark, pushed with the changes you recommended.

Aaron
  

Patch

diff --git a/src/srcfiles.cxx b/src/srcfiles.cxx
index c466b307..b6dd75ad 100644
--- a/src/srcfiles.cxx
+++ b/src/srcfiles.cxx
@@ -312,7 +312,6 @@  void zip_files()
   struct stat st;
   char buff[BUFFER_SIZE];
   int len;
-  int fd;
   #ifdef ENABLE_LIBDEBUGINFOD
   /* Initialize a debuginfod client.  */
   static unique_ptr <debuginfod_client, void (*)(debuginfod_client*)>
@@ -325,8 +324,10 @@  void zip_files()
   int missing_files = 0;
   for (const auto &pair : debug_sourcefiles)
   {
-    fd = -1;
+    int fd = -1;
     const std::string &file_path = pair.first;
+    struct archive_entry *entry = NULL;
+    string entry_name;
 
     /* Attempt to query debuginfod client to fetch source files.  */
     #ifdef ENABLE_LIBDEBUGINFOD
@@ -353,8 +354,8 @@  void zip_files()
     if (!no_backup)
     #endif /* ENABLE_LIBDEBUGINFOD */
       /* Files could not be located using debuginfod, search locally */
-      if (fd < 0)
-        fd = open(file_path.c_str(), O_RDONLY);
+    if (fd < 0)
+      fd = open(file_path.c_str(), O_RDONLY);
     if (fd < 0)
     {
       if (verbose)
@@ -371,11 +372,11 @@  void zip_files()
       missing_files++;
       if (verbose)
         cerr << "Error: Failed to get file status for " << file_path << ": " << strerror(errno) << endl;
-      continue;
+      goto next;
     }
-    struct archive_entry *entry = archive_entry_new();
+    entry = archive_entry_new();
     /* Removing first "/"" to make the path "relative" before zipping, otherwise warnings are raised when unzipping.  */
-    string entry_name = file_path.substr(file_path.find_first_of('/') + 1);
+    entry_name = file_path.substr(file_path.find_first_of('/') + 1);
     archive_entry_set_pathname(entry, entry_name.c_str());
     archive_entry_copy_stat(entry, &st);
     if (archive_write_header(a, entry) != ARCHIVE_OK)
@@ -385,7 +386,7 @@  void zip_files()
       missing_files++;
       if (verbose)
         cerr << "Error: failed to write header for " << file_path << ": " << archive_error_string(a) << endl;
-      continue;
+      goto next;
     }
 
     /* Write the file to the zip.  */
@@ -397,7 +398,7 @@  void zip_files()
       missing_files++;
       if (verbose)
         cerr << "Error: Failed to open file: " << file_path << ": " << strerror(errno) <<endl;
-      continue;
+      goto next;
     }
     while (len > 0)
     {
@@ -409,6 +410,8 @@  void zip_files()
       }
       len = read(fd, buff, sizeof(buff));
     }
+
+next:
     close(fd);
     archive_entry_free(entry);
   }