Bug 621 - Softening the variadic-macro requirement
Softening the variadic-macro requirement
Status: RESOLVED FIXED
Product: Sudo
Classification: Unclassified
Component: Sudo
1.8.7
PC Tru64 UNIX
: low normal
Assigned To: Todd C. Miller
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-11 13:54 MST by Daniel Richard G.
Modified: 2013-11-20 14:47 MST (History)
0 users

See Also:


Attachments
Patch against sudo hg tip (7.26 KB, patch)
2013-11-11 13:54 MST, Daniel Richard G.
Details | Diff
Follow-up patch against sudo hg tip (1.12 KB, patch)
2013-11-18 23:13 MST, Daniel Richard G.
Details | Diff
Tweaks (874 bytes, patch)
2013-11-19 00:49 MST, Daniel Richard G.
Details | Diff
Patch to prevent use of system [v]snprintf() on Tru64 (977 bytes, patch)
2013-11-19 01:09 MST, Daniel Richard G.
Details | Diff
Patch to enable *printf() function prototypes (928 bytes, patch)
2013-11-19 13:49 MST, Daniel Richard G.
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Richard G. 2013-11-11 13:54:12 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)
Comment 1 Todd C. Miller 2013-11-11 15:09:42 MST
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.
Comment 2 Daniel Richard G. 2013-11-11 19:37:42 MST
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().
Comment 3 Todd C. Miller 2013-11-12 11:04:40 MST
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).
Comment 4 Daniel Richard G. 2013-11-12 11:55:44 MST
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.
Comment 5 Todd C. Miller 2013-11-18 09:13:54 MST
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.
Comment 6 Daniel Richard G. 2013-11-18 23:13:34 MST
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.
Comment 7 Daniel Richard G. 2013-11-19 00:49:33 MST
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)
Comment 8 Daniel Richard G. 2013-11-19 01:09:10 MST
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.
Comment 9 Todd C. Miller 2013-11-19 10:06:15 MST
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?
Comment 10 Todd C. Miller 2013-11-19 10:41:59 MST
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.
Comment 11 Daniel Richard G. 2013-11-19 13:49:22 MST
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.
Comment 12 Todd C. Miller 2013-11-19 14:07:06 MST
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?
Comment 13 Daniel Richard G. 2013-11-19 15:51:27 MST
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.
Comment 14 Todd C. Miller 2013-11-19 16:06:16 MST
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.
Comment 15 Daniel Richard G. 2013-11-19 21:44:53 MST
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.)
Comment 16 Todd C. Miller 2013-11-20 07:22:49 MST
I added a definitino of RTLD_GLOBAL in the test for systems without it.
Comment 17 Daniel Richard G. 2013-11-20 14:47:08 MST
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!