Tails

kurono, would you be interested in having a look?

Ping?

I don't intend to block on the following concerns for this review'n'merge: the proposed code makes sense to me and I trust you that it does fix at least some instances of this bug.

Now, even if I merge this in a hurry for 3.10, I'll probably leave this ticket "In Progress" because I'm not sure the branch fixes all instances of the bug and I'm not convinced by the way the initially chosen approach to fix it.

I'm a bit confused wrt. what our logger should receive, i.e. a Python string or encoded bytes. E.g. I see self.log.debug(cmd_decoded) which suggests a Python string is prefered. But your branch changes many (but not all) calls to log.{warning,error,debug,etc.} so they're now passed encoded bytes, which seems consistent with the fact that our _ claims to return a "UTF-8 encoded bytestring". If our logger needs an encoded bytestring, fine, but then shouldn't self.live.log.exception(unicode(e)) be encoded to utf-8 too? Same for self.log.debug(cmd_decoded) and — I suspect — a bunch of other similar calls.

More generally, I'm not sure that manually adding "turn into a Unicode string and then encode to utf-8" everywhere is the best approach. I bet we'll always forget it occasionally. So I think we should automate this, probably by giving it a more clever stream handler or formatter in _setup_logger, that will use something like the _to_unicode and unicode_to_utf8 functions we already have, in order to support both all kinds of inputs.

What do you think?

It seems like the problem is not really with the logger, but with the encoding of the exception message, which can't be unicode at least one create a custom Exception that manages that.

I agree we have a problem with inconsistent decoded string vs. bytestring in exception handling and I'm glad you're on it! Your solution looks good to me. I'll test it in non-English locales in various situations where we raise an exception.

Wrt. the logger, I'm still a bit confused: could you please test & confirm that it works as expected regardless of whether we pass it a decoded string or a bytestring, in cases where it matters?

intrigeri wrote:

It seems like the problem is not really with the logger, but with the encoding of the exception message, which can't be unicode at least one create a custom Exception that manages that.

I agree we have a problem with inconsistent decoded string vs. bytestring in exception handling and I'm glad you're on it! Your solution looks good to me. I'll test it in non-English locales in various situations where we raise an exception.

Wrt. the logger, I'm still a bit confused: could you please test & confirm that it works as expected regardless of whether we pass it a decoded string or a bytestring, in cases where it matters?

Ok, I think I understand now, our code works as intended with decoded strings and encoding applied to decoded unicode bytestring for instance the following works:

self.log.info(_(u"Suppression du système d\'exploitation live existant"))
self.log.info(_(u"Suppression du système d\'exploitation live existant").encode("utf-8"))

but the following does not:

s = "Suppression du système d\'exploitation live existant" 
b = bytes(s, 'utf-8')
self.log.info(b)

and the normal string with unicode characters does not work:

self.log.info("Suppression du système d\'exploitation live existant")

I will take a look of all the instances when we call the logs to see if we are not doing it right.

Done. Please check Jenkins.

Done, test suite passes \o/

I'll merge, release and upload a new Tails Installer.

(FTR I'd rather the branch submitter to check Jenkins themselves before submitting, as per https://tails.boum.org/contribute/merge_policy/#submit.)

I'm a tiny bit worried that we have no reproducer here, so it's hard to tell for sure whether the proposed changes will fix real-world issues without introducing regressions. But kurono's analysis seems sound so I'll cross fingers and go ahead :)