Bugzilla – Bug 621
Softening the variadic-macro requirement
Last modified: 2013-11-20 14:47:08 MST
Created attachment 378 [details] Patch against sudo hg tip I need Sudo on a system whose vendor compiler cannot do variadic macros, and where GCC is not available. I've put together a set of changes that allow Sudo to be built with reduced [debugging] functionality when variadic macros are not supported, favoring graceful degradation over a hard stop. configure.in: * Don't exit with an error, but instead #define NO_VARIADIC_MACROS if the C compiler isn't up to snuff * Also, tell the user that we're checking for that include/fatal.h: * Make NO_VARIADIC_MACROS do the same thing as SUDO_ERROR_WRAP==0 * The fatal() -> fatal_nodebug() et al. redirects don't need to be variadic macros at all, because fatal_nodebug() is called with the exact same arguments as fatal() * Make fatal_nodebug() et al. into variable-argument functions, as they don't need to use __FILE__/__LINE__/__func__ include/sudo_debug.h: * If NO_VARIADIC_MACROS, then redirect sudo_debug_printf() to sudo_debug_printf_nvm(), a variable-argument function that just calls sudo_debug_vprintf2() with null source-line information common/fatal.c: * New fatal_nodebug() et al. functions common/sudo_debug.c: * New sudo_debug_printf_nvm() function (As an aside, I'm pretty sure that what vfatal() et al. in the SUDO_ERROR_WRAP!=0 case are doing is wrong---they're using a va_list twice. Have a look at http://bugs.debian.org/104325)
I'm surprised you can't use an older version of gcc. I seem to recall that either Digital or HP had a CD-ROM of freeware which included gcc. Unfortunately, the _nodebug versions need to be macros so that the gettext() calls happen after the call to warning_set_locale(). Otherwise, the warning may be displayed in the wrong locale. At one point I had the warning/fatal functions calling gettext() instead but there were issues with that. You are probably correct about vfatal() et al, they need a va_copy() in there.
The restriction is part technical (old version of Tru64, limited disk space) and part administrative (this system isn't exactly mine to do with as I please). So the gettext() call needs to happen after warning_set_locale(). How about a helper function that can be used for the _() macro? Like char * warning_gettext(char *in) { char *out; warning_set_locale(); out = gettext(in); warning_restore_locale(); return out; } #define _(s) warning_gettext(s) If _() has to do double duty for the user and sudoer locales, you could have two macros---say, U_() and S_(). It's a bigger change, but then the code becomes more explicit (as _() by itself is ambiguous), and this might even be able to subsume most of the calls to sudoers_setlocale().
We always want to print user warnings in the user's locale and log messages in the sudoers locale (C by default). Originally, warning/fatal did the gettext() call itself, but this was a problem for things using ngettext such as: warningx(ngettext("%d incorrect password attempt", "%d incorrect password attempts", tries), tries); There's already a special case for this in the logging code, so I suppose the warn/fatal code could have a similar hack (the debug code would also need it).
Could have another permutation of the macros that does ngettext(). So you'd have e.g. S_(), U_(), NS_(), NU_(). I see what you mean about the logging hack (INCORRECT_PASSWORD_ATTEMPT). Would you be willing to entertain a patch implementing this proposal? I think I should be able to do away with that.
I've checked in changes to use a gettext() wrapper when using warning/fatal that does the locale switch if needed. With that change warning2/fatal2 were renamed back to warning_nodebug/fatal_nodebug. I also checked in the remainder of your patch that was still relevant so hg tip should build for you now.
Created attachment 385 [details] Follow-up patch against sudo hg tip Tested the build on Tru64 again, and it's mostly there. This patch fixes a couple loose threads: * Need a definition of U_() for the --disable-nls case * On this system, there is no libintl.h, and no gettext() function. warning_gettext() is thus not used, but the call to gettext() causes linking to fail, so I put an "#ifdef HAVE_LIBINTL_H" around both definitions of the function. (This allows the build to succeed in both the --enable-nls and --disable-nls cases.) By the way, thanks for taking this on---I didn't figure you were implementing this change yourself.
Created attachment 386 [details] Tweaks This patch addresses a couple more minor issues: * compat/sig2str.c needs unistd.h, because Tru64 does #define SIGRTMIN (sysconf(131)) #define SIGRTMAX (sysconf(132)) * Made the no-variadic-macros warning pedantically correct (didn't catch this at the time as I wasn't in a pedantic mood)
Created attachment 387 [details] Patch to prevent use of system [v]snprintf() on Tru64 This is one more you may want to consider (and that I would have included in bug #624 if I'd caught it earlier). On Tru64, versions 4.0 and 5.1, [v]snprintf() returns the second argument (size) minus 1 if the size is too small for the output. Also, if the buffer is NULL, you get a segfault. While it's possible to work with that, it's completely non-standard, so I figure the easiest thing to do is blacklist it as broken in the configure script and use the bundled implementation instead.
I've committed all but the snprintf change. I'd like a more general solution to non-standard snprintf. Can you try https://www.gnu.org/software/autoconf-archive/ax_func_snprintf.html#ax_func_snprintf and verify that it detects the Tru64 snprintf as non-conforming?
Based on what you've written I think the check for snprintf will work for True64 so I've just committed it. If you could verify that hg tip works for you on Tru64 that would be great.
Created attachment 388 [details] Patch to enable *printf() function prototypes Yes, AX_FUNC_SNPRINTF correctly detects the broken functions on Tru64 v4.0 and 5.1. However, because HAVE_SNPRINTF et al. are defined, the prototypes in missing.h are not used, and you get a bunch of "In this statement, there is no prototype for 'snprintf'" warnings. Something like this patch should do the trick.
I'd rather not redefine snprintf in missing.h if it is already present in the system headers. Where are you getting warnings about a missing prototype?
I neglected to mention that Tru64 v4 has [v]snprintf() in the C library---that's what the configure script actually looks at, so the checks succeed---but no prototypes in the system headers. (In v5, stdio.h has them.) The configure script could do AC_CHECK_DECL(snprintf), but an easier solution may be to just use a different function name. Something like int sudo_snprintf(char *buf, blah blah blah); #undef snprintf #define snprintf sudo_snprintf That way, it doesn't matter if the system headers are missing a prototype, or have an incompatible declaration, or whatever. Just have to ensure that no system headers are included after that point.
I thought that might be the issue. I've renamed things to be rpl_snprintf, etc. for consistency with other functions in compat that may replace the libc versions. That should fix it.
Yep, that took care of it. The warnings are gone. There is one more thing needed for a clean build and successful (modulo check_symbols) test: cc -c -I../../../include -I../.. -I../../../plugins/sudoers -I../../../plugins/sudoers -I../../.. -DLIBDIR=\"/usr/local/lib\" -D_POSIX_PII -D_REENTRANT -D__STDC_WANT_LIB_EXT1__=1 -std1 -warnprotos -readonly_strings -fast -O4 -D_PATH_SUDOERS=\"/etc/sudoers\" -DSUDOERS_UID=0 -DSUDOERS_GID=0 -DSUDOERS_MODE=0440 -DLOCALEDIR=\"/usr/local/share/locale\" ../../../plugins/sudoers/regress/check_symbols/check_symbols.c cc: Error: ../../../plugins/sudoers/regress/check_symbols/check_symbols.c, line 81: In this statement, "RTLD_GLOBAL" is not declared. (undeclared) handle = dlopen(plugin_path, RTLD_LAZY|RTLD_GLOBAL); -------------------------------------------^ *** Exit 1 Stop. RTLD_GLOBAL doesn't exist at all on v4.0. (Apologies for not including this with all the general Tru64 stuff previously; I completely blanked on running the test suite for a few days.)
I added a definitino of RTLD_GLOBAL in the test for systems without it.
And that finishes it. Thank you for keeping these older systems in the game :-) Don't forget to update the docs (re variadic macros) before the next release!