Jan Stary wrote on Thu, Jan 10, 2019 at 08:24:58PM +0100:
> calendar imho doesn't need to setlocale(LC_TIME, ...)
> before and after strftime(3), as LC_TIME is ignored.
As it stands, this diff is not OK.
The calendar(1) program is also calling setlocale(LC_ALL, "")
from main, both in the parent and in the child.
And it is calling setlocale(LC_ALL, ...) with a non-trivial
argument read from the calendar file in io.c, function cal().
I believe a program should either not use setlocale(3) at all,
or it should use setlocale(3) correctly, in a safe way that is
portable, that is safe even when compiled on an operating system
other than OpenBSD.
So, as long as the program runs under setlocale(LC_ALL, "")
in general, you cannot remove setting of a safe locale around
strftime(3) just like that.
That said, i see at least three issues:
* In general, we don't want to use LC_ALL.
Even if some locale(1) usage is called for here, we probably
want to reduce that to the categorie(s) that actually matter.
Finding out which those are requires actual understanding of
* In general, we don't want to run with a locale unless there
is a complling reason to.
It appears locale(1) functionality is at least used for the
BODUN= date calculation mode for Cyrillic calendars (whatever
that is), so i doubt setlocale(3) can be removed completely
without loss of functionality.
But maybe it would be better to call setlocale(3) only at the
one place where it is actually used (unless there are others)
and restore to "C" just afterwards, rather than having it set
during the whole program and having to worry about security
at places like the one you are just proposing to make insecure?
Not sure, it depends on what the program is doing throughout.
* Using setlocale(LC_TIME, "") to restore the previous
locale looks fragile. Normally, to save and restore a locale,
you have to call setlocale(category, NULL), copy the return
value, and pass it back to setlocale(3) to restore.
It may not matter too much here because the program
may not modify its environment, but this is still
a dangerous idiom.
* The fact that part of the program runs under a locale taken
from the environment and other parts run under a locale
taken from the calendar file looks fishy. What are the
consequences if both locales conflict? Are there bugs
hiding in there?
So, if someone wants to improve this program, there is some real
work to do. I'm not sure i want to tackle it right now - i rarely
use this program, and at this point, even though i see some ugliness,
i see no strong indication that what there is now might constitute
a security risk.