Another one I'd like to check consensus on, before disappearing too far
down the rabbit-hole.
The ap_set_*_slot directive function initializers throw away type safety
completely since you can pass anything to APR_OFFSETOF(x,y) and it's
cast to void * regardless. I've seen one bug because of this in a
third-party module which was using a bool [sizeof(char)] rather than an
int with ap_set_int_slot. r1876823 has another example.
You can get away with "minor" errors like that on little-endian
platforms since the least significant bytes are in the "right" place
even if the type is wrong. So bugs don't show up until customers run
your code on, just as an example, IBM hardware! ;)
I don't see a way to fix all the code with hard-coded APR_OFFSETOF(),
but we can offer some alternative wrapper macros which get rid of
OFFSETOF and check the types at compile-time with some gcc builtin
magic. PoC attached.
The failure will look like this one from the mod_proxy_html case:
In file included from mod_proxy_html.c:55:
/home/jorton/src/asf/httpd-git/include/http_config.h:162:5: error: void value
not ignored as it ought to be
162 | __builtin_choose_expr(sizeof(actual) == sizeof(expected), result,
(void)0)
| ^~~~~~~~~~~~~~~~~~~~~
/home/jorton/src/asf/httpd-git/include/http_config.h:169:13: note: in expansion
of macro ‘AP_INIT_CHECKED_TYPE’
169 | AP_INIT_CHECKED_TYPE(((structname *)0)->fieldname, int,
\
| ^~~~~~~~~~~~~~~~~~~~
mod_proxy_html.c:1316:5: note: in expansion of macro ‘AP_INIT_TAKE1_INT_SLOT’
1316 | AP_INIT_TAKE1_INT_SLOT("ProxyHTMLBufSize", proxy_html_conf, bufsz,
| ^~~~~~~~~~~~~~~~~~~~~~
which is not totally obvious but at least it's a failure.
Opinions?
Regards, Joe
diff --git a/include/http_config.h b/include/http_config.h
index 2aac6d4325..0c32d94b3c 100644
--- a/include/http_config.h
+++ b/include/http_config.h
@@ -155,6 +155,31 @@ typedef union {
# define AP_INIT_FLAG(directive, func, mconfig, where, help) \
{ directive, { .flag=func }, mconfig, where, FLAG, help }
+#ifdef __GNUC__
+/* Expands to a compile-time error (void)0 if size of actual and
+ * expected arguments do not match, otherwise expands to results. */
+# define AP_INIT_CHECKED_TYPE(actual, expected, result) \
+ __builtin_choose_expr(sizeof(actual) == sizeof(expected), result, (void)0)
+#else
+# define AP_INIT_CHECKED_TYPE(actual, expected, result) result
+#endif
+
+# define AP_INIT_TAKE1_INT_SLOT(directive, structname, fieldname, where, help)
\
+ { directive, { .take1=ap_set_int_slot }, \
+ AP_INIT_CHECKED_TYPE(((structname *)0)->fieldname, int, \
+ (void *)APR_OFFSETOF(structname, fieldname)),
\
+ where, TAKE1, help }
+# define AP_INIT_FLAG_SLOT(directive, structname, fieldname, where, help) \
+ { directive, { .flag=ap_set_flag_slot }, \
+ AP_INIT_CHECKED_TYPE(((structname *)0)->fieldname, int, \
+ (void *)APR_OFFSETOF(structname, fieldname)), \
+ where, FLAG, help }
+# define AP_INIT_FLAG_CHAR_SLOT(directive, structname, fieldname, where, help)
\
+ { directive, { .flag=ap_set_flag_slot_char }, \
+ AP_INIT_CHECKED_TYPE(((structname *)0)->fieldname, char, \
+ (void *)APR_OFFSETOF(structname, fieldname)), \
+ where, FLAG, help }
+
#else /* AP_HAVE_DESIGNATED_INITIALIZER */
typedef const char *(*cmd_func) ();
@@ -193,6 +218,15 @@ typedef const char *(*cmd_func) ();
{ directive, func, mconfig, where, TAKE3, help }
# define AP_INIT_FLAG(directive, func, mconfig, where, help) \
{ directive, func, mconfig, where, FLAG, help }
+# define AP_INIT_TAKE1_INT_SLOT(directive, structname, fieldname, where, help)
\
+ { directive, ap_set_int_slot }, (void *)APR_OFFSETOF(structname,
fieldname)), \
+ where, TAKE1, help }
+# define AP_INIT_FLAG_SLOT(directive, structname, fieldname, where, help) \
+ { directive, ap_set_flag_slot, (void *)APR_OFFSETOF(structname,
fieldname), \
+ where, FLAG, help }
+# define AP_INIT_FLAG_CHAR_SLOT(directive, structname, fieldname, where, help)
\
+ { directive, ap_set_flag_slot_char, APR_OFFSETOF(structname, fieldname)), \
+ where, FLAG, help }
#endif /* AP_HAVE_DESIGNATED_INITIALIZER */
diff --git a/modules/aaa/mod_authnz_ldap.c b/modules/aaa/mod_authnz_ldap.c
index 08f5fa1bc9..a0d55aa8db 100644
--- a/modules/aaa/mod_authnz_ldap.c
+++ b/modules/aaa/mod_authnz_ldap.c
@@ -1755,12 +1755,12 @@ static const command_rec authnz_ldap_cmds[] =
AP_INIT_TAKE1("AuthLDAPBindPassword", set_bind_password, NULL, OR_AUTHCFG,
"Password to use to bind to LDAP server. If not provided,
will do an anonymous bind."),
- AP_INIT_FLAG("AuthLDAPBindAuthoritative", ap_set_flag_slot,
- (void *)APR_OFFSETOF(authn_ldap_config_t,
bind_authoritative), OR_AUTHCFG,
- "Set to 'on' to return failures when user-specific bind
fails - defaults to on."),
+ AP_INIT_FLAG_SLOT("AuthLDAPBindAuthoritative", authn_ldap_config_t,
bind_authoritative,
+ OR_AUTHCFG,
+ "Set to 'on' to return failures when user-specific bind
fails - defaults to on."),
- AP_INIT_FLAG("AuthLDAPRemoteUserIsDN", ap_set_flag_slot,
- (void *)APR_OFFSETOF(authn_ldap_config_t, user_is_dn),
OR_AUTHCFG,
+ AP_INIT_FLAG_SLOT("AuthLDAPRemoteUserIsDN", authn_ldap_config_t,
user_is_dn,
+ OR_AUTHCFG,
"Set to 'on' to set the REMOTE_USER environment variable to
be the full "
"DN of the remote user. By default, this is set to off,
meaning that "
"the REMOTE_USER variable will contain whatever value the
remote user sent."),
@@ -1771,8 +1771,8 @@ static const command_rec authnz_ldap_cmds[] =
"contents of this attribute in the REMOTE_USER "
"environment variable."),
- AP_INIT_FLAG("AuthLDAPCompareDNOnServer", ap_set_flag_slot,
- (void *)APR_OFFSETOF(authn_ldap_config_t,
compare_dn_on_server), OR_AUTHCFG,
+ AP_INIT_FLAG_SLOT("AuthLDAPCompareDNOnServer", authn_ldap_config_t,
+ compare_dn_on_server, OR_AUTHCFG,
"Set to 'on' to force auth_ldap to do DN compares (for the
\"require dn\" "
"directive) using the server, and set it 'off' to do the
compares locally "
"(at the expense of possible false matches). See the
documentation for "
@@ -1813,20 +1813,17 @@ static const command_rec authnz_ldap_cmds[] =
"The prefix to add to environment variables set during "
"successful authorization, default '" AUTHZ_PREFIX "'"),
- AP_INIT_FLAG("AuthLDAPInitialBindAsUser", ap_set_flag_slot,
- (void *)APR_OFFSETOF(authn_ldap_config_t,
initial_bind_as_user), OR_AUTHCFG,
+ AP_INIT_FLAG_SLOT("AuthLDAPInitialBindAsUser", authn_ldap_config_t,
initial_bind_as_user, OR_AUTHCFG,
"Set to 'on' to perform the initial DN lookup with the basic
auth credentials "
"instead of anonymous or hard-coded credentials"),
AP_INIT_TAKE2("AuthLDAPInitialBindPattern", set_bind_pattern, NULL,
OR_AUTHCFG,
"The regex and substitution to determine a username that
can bind based on an HTTP basic auth username"),
- AP_INIT_FLAG("AuthLDAPSearchAsUser", ap_set_flag_slot,
- (void *)APR_OFFSETOF(authn_ldap_config_t, search_as_user),
OR_AUTHCFG,
+ AP_INIT_FLAG_SLOT("AuthLDAPSearchAsUser", authn_ldap_config_t,
search_as_user, OR_AUTHCFG,
"Set to 'on' to perform authorization-based searches with
the users credentials, when this module "
"has also performed authentication. Does not affect nested
groups lookup."),
- AP_INIT_FLAG("AuthLDAPCompareAsUser", ap_set_flag_slot,
- (void *)APR_OFFSETOF(authn_ldap_config_t, compare_as_user),
OR_AUTHCFG,
+ AP_INIT_FLAG_SLOT("AuthLDAPCompareAsUser", authn_ldap_config_t,
compare_as_user, OR_AUTHCFG,
"Set to 'on' to perform authorization-based compares with
the users credentials, when this module "
"has also performed authentication. Does not affect nested
groups lookups."),
{NULL}
diff --git a/modules/aaa/mod_authz_dbd.c b/modules/aaa/mod_authz_dbd.c
index e1bb6232cf..73f34d355c 100644
--- a/modules/aaa/mod_authz_dbd.c
+++ b/modules/aaa/mod_authz_dbd.c
@@ -100,9 +100,8 @@ static const char *authz_dbd_prepare(cmd_parms *cmd, void
*cfg,
}
static const command_rec authz_dbd_cmds[] = {
- AP_INIT_FLAG("AuthzDBDLoginToReferer", ap_set_flag_slot,
- (void*)APR_OFFSETOF(authz_dbd_cfg, redirect), ACCESS_CONF,
- "Whether to redirect to referer on successful login"),
+ AP_INIT_FLAG_SLOT("AuthzDBDLoginToReferer", authz_dbd_cfg, redirect,
+ ACCESS_CONF, "Whether to redirect to referer on
successful login"),
AP_INIT_TAKE1("AuthzDBDQuery", authz_dbd_prepare,
(void*)APR_OFFSETOF(authz_dbd_cfg, query), ACCESS_CONF,
"SQL query for DBD Authz or login"),
diff --git a/modules/filters/mod_include.c b/modules/filters/mod_include.c
index 584d8fb311..02cb8136cb 100644
--- a/modules/filters/mod_include.c
+++ b/modules/filters/mod_include.c
@@ -4194,17 +4194,14 @@ static const command_rec includes_cmds[] =
"SSI End String Tag"),
AP_INIT_TAKE1("SSIUndefinedEcho", set_undefined_echo, NULL, OR_ALL,
"String to be displayed if an echoed variable is undefined"),
- AP_INIT_FLAG("SSILegacyExprParser", ap_set_flag_slot_char,
- (void *)APR_OFFSETOF(include_dir_config, legacy_expr),
+ AP_INIT_FLAG_CHAR_SLOT("SSILegacyExprParser", include_dir_config,
legacy_expr,
OR_LIMIT,
"Whether to use the legacy expression parser compatible "
"with <= 2.2.x. Limited to 'on' or 'off'"),
- AP_INIT_FLAG("SSILastModified", ap_set_flag_slot_char,
- (void *)APR_OFFSETOF(include_dir_config, lastmodified),
+ AP_INIT_FLAG_CHAR_SLOT("SSILastModified", include_dir_config, lastmodified,
OR_LIMIT, "Whether to set the last modified header or
respect "
"an existing header. Limited to 'on' or 'off'"),
- AP_INIT_FLAG("SSIEtag", ap_set_flag_slot_char,
- (void *)APR_OFFSETOF(include_dir_config, etag),
+ AP_INIT_FLAG_CHAR_SLOT("SSIEtag", include_dir_config, etag,
OR_LIMIT, "Whether to allow the generation of ETags within
the server. "
"Existing ETags will be preserved. Limited to 'on' or
'off'"),
{NULL}
diff --git a/modules/filters/mod_proxy_html.c b/modules/filters/mod_proxy_html.c
index c50b07a0c1..1308d557af 100644
--- a/modules/filters/mod_proxy_html.c
+++ b/modules/filters/mod_proxy_html.c
@@ -107,7 +107,7 @@ typedef struct {
const char *doctype;
const char *etag;
unsigned int flags;
- size_t bufsz;
+ int bufsz;
apr_hash_t *links;
apr_array_header_t *events;
const char *charset_out;
@@ -1304,27 +1304,21 @@ static const command_rec proxy_html_cmds[] = {
RSRC_CONF|ACCESS_CONF, "(HTML|XHTML) [Legacy]"),
AP_INIT_ITERATE("ProxyHTMLFixups", set_flags, NULL,
RSRC_CONF|ACCESS_CONF, "Options are lowercase, dospath"),
- AP_INIT_FLAG("ProxyHTMLMeta", ap_set_flag_slot,
- (void*)APR_OFFSETOF(proxy_html_conf, metafix),
+ AP_INIT_FLAG_SLOT("ProxyHTMLMeta", proxy_html_conf, metafix,
RSRC_CONF|ACCESS_CONF, "Fix META http-equiv elements"),
- AP_INIT_FLAG("ProxyHTMLInterp", ap_set_flag_slot,
- (void*)APR_OFFSETOF(proxy_html_conf, interp),
+ AP_INIT_FLAG_SLOT("ProxyHTMLInterp", proxy_html_conf, interp,
RSRC_CONF|ACCESS_CONF,
"Support interpolation and conditions in URLMaps"),
- AP_INIT_FLAG("ProxyHTMLExtended", ap_set_flag_slot,
- (void*)APR_OFFSETOF(proxy_html_conf, extfix),
+ AP_INIT_FLAG_SLOT("ProxyHTMLExtended", proxy_html_conf, extfix,
RSRC_CONF|ACCESS_CONF, "Map URLs in Javascript and CSS"),
- AP_INIT_FLAG("ProxyHTMLStripComments", ap_set_flag_slot,
- (void*)APR_OFFSETOF(proxy_html_conf, strip_comments),
+ AP_INIT_FLAG_SLOT("ProxyHTMLStripComments", proxy_html_conf,
strip_comments,
RSRC_CONF|ACCESS_CONF, "Strip out comments"),
- AP_INIT_TAKE1("ProxyHTMLBufSize", ap_set_int_slot,
- (void*)APR_OFFSETOF(proxy_html_conf, bufsz),
+ AP_INIT_TAKE1_INT_SLOT("ProxyHTMLBufSize", proxy_html_conf, bufsz,
RSRC_CONF|ACCESS_CONF, "Buffer size"),
AP_INIT_TAKE1("ProxyHTMLCharsetOut", ap_set_string_slot,
(void*)APR_OFFSETOF(proxy_html_conf, charset_out),
RSRC_CONF|ACCESS_CONF, "Usage: ProxyHTMLCharsetOut charset"),
- AP_INIT_FLAG("ProxyHTMLEnable", ap_set_flag_slot,
- (void*)APR_OFFSETOF(proxy_html_conf, enabled),
+ AP_INIT_FLAG_SLOT("ProxyHTMLEnable", proxy_html_conf, enabled,
RSRC_CONF|ACCESS_CONF,
"Enable proxy-html and xml2enc filters"),
{ NULL }
diff --git a/modules/generators/mod_autoindex.c
b/modules/generators/mod_autoindex.c
index f977b88324..129ecada97 100644
--- a/modules/generators/mod_autoindex.c
+++ b/modules/generators/mod_autoindex.c
@@ -584,8 +584,7 @@ static const command_rec autoindex_cmds[] =
"{Ascending,Descending} {Name,Size,Description,Date}"),
AP_INIT_ITERATE("IndexIgnore", add_ignore, NULL, DIR_CMD_PERMS,
"one or more file extensions"),
- AP_INIT_FLAG("IndexIgnoreReset", ap_set_flag_slot,
- (void *)APR_OFFSETOF(autoindex_config_rec, ign_noinherit),
+ AP_INIT_FLAG_SLOT("IndexIgnoreReset", autoindex_config_rec, ign_noinherit,
DIR_CMD_PERMS,
"Reset the inherited list of IndexIgnore filenames"),
AP_INIT_ITERATE2("AddDescription", add_desc, BY_PATH, DIR_CMD_PERMS,
diff --git a/modules/http/mod_mime.c b/modules/http/mod_mime.c
index 3cf907148c..3ddca13fdf 100644
--- a/modules/http/mod_mime.c
+++ b/modules/http/mod_mime.c
@@ -471,8 +471,7 @@ static const command_rec mime_cmds[] =
"one or more file extensions"),
AP_INIT_TAKE1("TypesConfig", set_types_config, NULL, RSRC_CONF,
"the MIME types config file"),
- AP_INIT_FLAG("ModMimeUsePathInfo", ap_set_flag_slot,
- (void *)APR_OFFSETOF(mime_dir_config, use_path_info), ACCESS_CONF,
+ AP_INIT_FLAG_SLOT("ModMimeUsePathInfo", mime_dir_config, use_path_info,
ACCESS_CONF,
"Set to 'yes' to allow mod_mime to use path info for type checking"),
AP_INIT_ITERATE("MimeOptions",
add_mime_options,
diff --git a/modules/mappers/mod_alias.c b/modules/mappers/mod_alias.c
index 569d331e7f..3c64481942 100644
--- a/modules/mappers/mod_alias.c
+++ b/modules/mappers/mod_alias.c
@@ -712,8 +712,7 @@ static const command_rec alias_cmds[] =
AP_INIT_TAKE2("RedirectPermanent", add_redirect2,
(void *) HTTP_MOVED_PERMANENTLY, OR_FILEINFO,
"a document to be redirected, then the destination URL"),
- AP_INIT_FLAG("RedirectRelative", ap_set_flag_slot,
- (void*)APR_OFFSETOF(alias_dir_conf, allow_relative),
OR_FILEINFO,
+ AP_INIT_FLAG_SLOT("RedirectRelative", alias_dir_conf, allow_relative,
OR_FILEINFO,
"Set to ON to allow relative redirect targets to be issued
as-is"),
{NULL}
diff --git a/modules/mappers/mod_speling.c b/modules/mappers/mod_speling.c
index 2ed65eb810..3f4cb8f32b 100644
--- a/modules/mappers/mod_speling.c
+++ b/modules/mappers/mod_speling.c
@@ -103,15 +103,12 @@ static void *create_mconfig_for_directory(apr_pool_t *p,
char *dir)
*/
static const command_rec speling_cmds[] =
{
- AP_INIT_FLAG("CheckSpelling", ap_set_flag_slot,
- (void*)APR_OFFSETOF(spconfig, enabled), OR_OPTIONS,
- "whether or not to fix miscapitalized/misspelled requests"),
- AP_INIT_FLAG("CheckCaseOnly", ap_set_flag_slot,
- (void*)APR_OFFSETOF(spconfig, check_case_only), OR_OPTIONS,
- "whether or not to fix only miscapitalized requests"),
- AP_INIT_FLAG("CheckBasenameMatch", ap_set_flag_slot,
- (void*)APR_OFFSETOF(spconfig, check_basename_match),
OR_OPTIONS,
- "whether or not to fix files with the same base name"),
+ AP_INIT_FLAG_SLOT("CheckSpelling", spconfig, enabled, OR_OPTIONS,
+ "whether or not to fix miscapitalized/misspelled
requests"),
+ AP_INIT_FLAG_SLOT("CheckCaseOnly", spconfig, check_case_only, OR_OPTIONS,
+ "whether or not to fix only miscapitalized requests"),
+ AP_INIT_FLAG_SLOT("CheckBasenameMatch", spconfig, check_basename_match,
OR_OPTIONS,
+ "whether or not to fix files with the same base name"),
{ NULL }
};
diff --git a/modules/proxy/mod_proxy.c b/modules/proxy/mod_proxy.c
index 71fae13edc..8d90c8e9d0 100644
--- a/modules/proxy/mod_proxy.c
+++ b/modules/proxy/mod_proxy.c
@@ -2716,8 +2716,7 @@ static const command_rec proxy_cmds[] =
"a scheme, partial URL or '*' and a proxy server"),
AP_INIT_TAKE2("ProxyRemoteMatch", add_proxy_regex, NULL, RSRC_CONF,
"a regex pattern and a proxy server"),
- AP_INIT_FLAG("ProxyPassInterpolateEnv", ap_set_flag_slot_char,
- (void*)APR_OFFSETOF(proxy_dir_conf, interpolate_env),
+ AP_INIT_FLAG_CHAR_SLOT("ProxyPassInterpolateEnv", proxy_dir_conf,
interpolate_env,
RSRC_CONF|ACCESS_CONF, "Interpolate Env Vars in reverse Proxy") ,
AP_INIT_RAW_ARGS("ProxyPass", add_pass_noregex, NULL,
RSRC_CONF|ACCESS_CONF,
"a virtual path and a URL"),
