srcfiles.cxx: Prevent fd and entry leak
Commit Message
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
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
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
@@ -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);
}