koji

Clone

#3423 Fix rpm_hdr_size file closing
Merged 2 years ago by tkopecek. Opened 2 years ago by puiterwijk.
puiterwijk/koji fix_rpm_hdr_size_fileclose  into  master

file modified
+1 -1 
@@ -648,7 +648,7 @@ 
 

      # add eight bytes for section header
 

      hdrsize = hdrsize + 8
 

  

-     if not isinstance(f, six.string_types):
 

+     if isinstance(f, six.string_types):
 

          fo.close()
 

      return hdrsize

The current code will try to close a passed in file object, and will not
try to close a file object it opened when a string is passed in.
This results in either a leaked file object, or a file object that was
passed in to be closed after the function is called.

Signed-off-by: Patrick Uiterwijk patrick@puiterwijk.org

rebased onto 83d27abe7b2c45eca14540508140b6c9eeae9616
2 years ago

rebased onto 8fc27c8
2 years ago

Fun fact: this bug has been in here since the initial code drop.

:thumbsup: (issue #3424)

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready
2 years ago

afaict, we never call this function with a file object, only with a filename. (Which must be the case since it would definitely error in the file object case)

In the string case, the created file object becomes unreferenced quickly, so likely closed by gc but not guaranteed to be timely, which I guess is why we've never run across leaking behavior from this (also, none of the processes that call this are normally long-lived).

anyway, thanks for the fix!

:thumbsup:

I hit this a few months ago already when doing head-signing for RHEL, but I forgot to file this (I am using Koji's functions to get headers etc), and at the time I just worked around it by passing paths into Koji.
I now again hit it when doing the head-signing work for Fedora, and now I decided to just fix it, since my current code tries to write as little as possible to disk.

So basically, this is indeed not an issue within the Koji client/server usecases.
And yeah GC does close the file descriptors, it just logs a warning.

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-ready
- Pull-request tagged with: no_qe
2 years ago

pretty please pagure-ci rebuild
2 years ago

Commit e5e59c1 fixes this pull-request

Pull-Request has been merged by tkopecek
2 years ago
Metadata
None

no_qe

Changes Summary 1
+1 -1
file changed
koji/__init__.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.