https://bugzilla.redhat.com/show_bug.cgi?id=1202604 --- Comment #10 from Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> --- (In reply to Stephen Gallagher from comment #9) > (In reply to Zbigniew Jędrzejewski-Szmek from comment #6) > > It seems that tempfile.NamedTemporaryFile would do the right thing without > > requiring all this manual cleanup: > > > > The reason I'm using the tempfile.mkstemp() implementation is this: > > "Creates a temporary file in the most secure manner possible. There are no > race conditions in the file’s creation, assuming that the platform properly > implements the os.O_EXCL flag for os.open()." TemporaryFile and NamedTemporaryFile and mkstemp share the implementation, so there's no difference in security, only in the way that file is automatically deleted. (In reply to Stephen Gallagher from comment #8) > Well, I was trying to address a potential containerized case where either > python release might be available. If I was only going to include the script > for the default python, it doesn't make sense to build both (this module > isn't really intended to be imported by other scripts, though it could be). > So if you're building a Dockerfile and want the image to generate a > certificate on startup, you probably want to be able to use whichever > version of the script matches the version of python already in the > environment. But this actually makes things more complicated for the user, because if the have only the non-default version installed, there's no /usr/bin/sscg. So maybe there shouldn't be packages for both versions, just one, always providing /usr/bin/sscg, and internally it should use either python2 or python3, whichever one is the default for this Fedora version. > > In the python code, a bare try: ... except: (without any exception na > > after "except") is used. In some places the exception is re-raised, but in > > others not. This is a problem because the script might be ended with ^C (or > > a signal) and silently "swallow" the exception. The script should always > > exit after being interrupted. It also should not print an error message like > > "Failed to write..." when killed. > Ah, where did I do that? It wasn't intentional. I'm not sure about what one specifically you're asking... "bare" except is used almost everywhere. In src/sscg/main.py there is this: try: os.unlink(file) except: # Nothing we can do if we get an error pass > > It seems that gettext is called on formatted arguments: > > _("Could not write to {0}. Error: {1}".format(...)) > > but it should be before: > > _("Could not write to {0}. Error: {1}").format(...) > > > > Typo, thanks for catching it. > > > Why open the temp file in text mode, then decode and re-encode the > > certificate. Maybe it would be simpler to open it in binary mode and skip > > the decode step? > > > > I don't do I? I thought the default was binary mode, which is why I was > decoding the UTF-8 to the binary representation. Or have I confused myself? > (I don't like dealing with encodings...) Yes, binary is the default. But decoding goes from bytes to unicode (i.e. text, i.e. str in Python 3). Encoding goes from text (i.e. str) to a specific encoding, for example UTF-8, and the bytes type. So you were writing text to a binary file. This actually does not work in Python 3: >>> f = tempfile.NamedTemporaryFile() >>> f.write('blah') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib64/python3.4/tempfile.py", line 399, in func_wrapper return func(*args, **kwargs) TypeError: 'str' does not support the buffer interface although it does work under Python 2. > > It seems that this will be usable a library. I think that calling sys.exit() > > should not be done, except in main. Errors should not be printed either. > > Simply raising an exception with an appropriately formatted message and > > letting the caller handle it would nicely simplify things. > > It's not supposed to be used directly as a library, only as a script. OK. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review