"index off;" directive?
Fabiano Furtado
fabianofurtado at gmail.com
Wed May 6 01:05:36 UTC 2026
HI!
On Tue, May 5, 2026 at 6:16 AM Maxim Dounin wrote:
>
> Hello!
> ...
> Good.
>
> > While searching for a code pattern in the freenginx codebase to guide
> > my implementation, I accidentally discovered a bug in
> > ngx_http_log_module (ngx_http_log_open_file_cache) when testing the
> > "off" parameter in different positions. For example, the configuration
> > "open_log_file_cache off max=1;" should fail, but it does not.
>
> Most of the named argument directive parsers are somewhat sloppy -
> in particular, they fail to detect duplicate arguments (such as in
> "open_file_cache max=1 max=2;") and often fail to detect
> conflicting arguments, as in your example.
OK.
> > I hope this patch works well! Thank you, again!
> > Fabiano Furtado
>
> > diff -r f347a195b373 src/http/modules/ngx_http_index_module.c
> > --- a/src/http/modules/ngx_http_index_module.c Thu Apr 30 07:25:52 2026 +0300
> > +++ b/src/http/modules/ngx_http_index_module.c Sun May 03 22:15:23 2026 -0300
> > @@ -20,6 +20,7 @@
> > typedef struct {
> > ngx_array_t *indices; /* array of ngx_http_index_t */
> > size_t max_index_len;
> > + ngx_flag_t off;
>
> The "ngx_flag_t" type is used for configuration directives with
> the ngx_conf_set_flag_slot() handler. There is no need to use
> this type for fields which aren't with ngx_conf_set_flag_slot().
>
> In this particular case, just one bit should be enough. But since
> there are no other bit fields in the structure, there is no need
> to declare it as a bit field, as there will be no memory benefits,
> and might be performance degradation. Existing approach for such
> cases is to use the ngx_uint_t type and provide corresponding bit
> field type in a comment, e.g.:
>
> ngx_uint_t off; /* unsigned:1 */
>
> You can see an example in the access log module you've mentioned.
>
> > } ngx_http_index_loc_conf_t;
> >
> >
> > @@ -109,6 +110,12 @@
> > ngx_http_index_loc_conf_t *ilcf;
> > ngx_http_script_len_code_pt lcode;
> >
> > + ilcf = ngx_http_get_module_loc_conf(r, ngx_http_index_module);
> > +
> > + if (ilcf->off) {
> > + return NGX_DECLINED;
> > + }
> > +
> > if (r->uri.data[r->uri.len - 1] != '/') {
> > return NGX_DECLINED;
> > }
>
> You've probably missed the comment about this code in the previous
> review:
>
> > > I believe checking r->uri before and method before even trying to
> > > access the configuration is optimal from performance point of
> > > view, and this is what index module does now, and other modules do
> > > as well (see autoindex or random index for some examples). There
> > > is no need to change this order.
Sorry! :(
> > @@ -117,7 +124,6 @@
> > return NGX_DECLINED;
> > }
> >
> > - ilcf = ngx_http_get_module_loc_conf(r, ngx_http_index_module);
> > clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
> >
> > allocated = 0;
> > @@ -396,6 +402,7 @@
> >
> > conf->indices = NULL;
> > conf->max_index_len = 0;
> > + conf->off = 0;
> >
> > return conf;
> > }
> > @@ -409,31 +416,34 @@
> >
> > ngx_http_index_t *index;
> >
> > - if (conf->indices == NULL) {
> > - conf->indices = prev->indices;
> > - conf->max_index_len = prev->max_index_len;
> > + if (conf->indices || conf->off) {
> > + return NGX_CONF_OK;
> > + }
> > +
> > + conf->indices = prev->indices;
> > + conf->max_index_len = prev->max_index_len;
> > + conf->off = prev->off;
> > +
> > + if (conf->indices || conf->off) {
> > + return NGX_CONF_OK;
> > }
>
> In general, an early return from a merge function is better
> avoided: it makes extending the function non-trivial, so
> introducing additional configuration directives will be
> complicated.
I based my approach on the merge function in the log module, but I understand.
> > + conf->indices = ngx_array_create(cf->pool, 1, sizeof(ngx_http_index_t));
> > if (conf->indices == NULL) {
> > - conf->indices = ngx_array_create(cf->pool, 1, sizeof(ngx_http_index_t));
> > - if (conf->indices == NULL) {
> > - return NGX_CONF_ERROR;
> > - }
> > + return NGX_CONF_ERROR;
> > + }
> >
> > - index = ngx_array_push(conf->indices);
> > - if (index == NULL) {
> > - return NGX_CONF_ERROR;
> > - }
> > + index = ngx_array_push(conf->indices);
> > + if (index == NULL) {
> > + return NGX_CONF_ERROR;
> > + }
> >
> > - index->name.len = sizeof(NGX_HTTP_DEFAULT_INDEX);
> > - index->name.data = (u_char *) NGX_HTTP_DEFAULT_INDEX;
> > - index->lengths = NULL;
> > - index->values = NULL;
> > + index->name.len = sizeof(NGX_HTTP_DEFAULT_INDEX);
> > + index->name.data = (u_char *) NGX_HTTP_DEFAULT_INDEX;
> > + index->lengths = NULL;
> > + index->values = NULL;
> >
> > - conf->max_index_len = sizeof(NGX_HTTP_DEFAULT_INDEX);
> > -
> > - return NGX_CONF_OK;
> > - }
> > + conf->max_index_len = sizeof(NGX_HTTP_DEFAULT_INDEX);
> >
> > return NGX_CONF_OK;
> > }
> > @@ -470,6 +480,28 @@
> > ngx_http_index_t *index;
> > ngx_http_script_compile_t sc;
> >
> > + value = cf->args->elts;
> > +
> > + if (ngx_strcmp(value[1].data, "off") == 0) {
> > + if (cf->args->nelts == 2) {
> > + ilcf->off = 1;
> > + return NGX_CONF_OK;
> > + }
> > +
> > + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > + "invalid parameter \"%V\"", &value[2]);
> > + return NGX_CONF_ERROR;
> > + }
> > +
> > + for (i = 2; i < cf->args->nelts; i++) {
> > +
> > + if (ngx_strcmp(value[i].data, "off") == 0) {
> > + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > + "parameter \"off\" must be declared first");
> > + return NGX_CONF_ERROR;
> > + }
> > + }
>
> I believe there is no need for an additional loop here, we already
> have one (and it already does a couple of similar checks).
OK.
> > +
> > if (ilcf->indices == NULL) {
> > ilcf->indices = ngx_array_create(cf->pool, 2, sizeof(ngx_http_index_t));
> > if (ilcf->indices == NULL) {
> > @@ -477,8 +509,6 @@
> > }
> > }
> >
> > - value = cf->args->elts;
> > -
> > for (i = 1; i < cf->args->nelts; i++) {
> >
> > if (value[i].data[0] == '/' && i != cf->args->nelts - 1) {
>
> Overall, looking through a couple of variants I tend to think that
> using licf->indices == NULL as a special value would be optimal
> here. Suggested patch below.
Yeah! Using indices, you optimize memory and have one less variable to
manage in the module.
KISS principle! ;)
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1777971559 -10800
> # Tue May 05 11:59:19 2026 +0300
> # Node ID e8bd179e5e7be4e6422918dbc6cace143de7b8ce
> # Parent f347a195b373c1d118fd8a929886692f052e8d4b
> Index: added "index off;".
>
> This might be useful to save a syscall if index files should not be used,
> for example, when autoindex is expected to be used instead.
>
> Prodded by Fabiano Furtado.
Thank you very much for the mention!
> diff --git a/src/http/modules/ngx_http_index_module.c b/src/http/modules/ngx_http_index_module.c
> --- a/src/http/modules/ngx_http_index_module.c
> +++ b/src/http/modules/ngx_http_index_module.c
> ...
> return NGX_CONF_ERROR;
> }
I'll test your patch, though I'm confident it's already working perfectly.
Thanks for your resilience, patience, time and help.
I still couldn't produce a working patch, but I've learned a lot, and
I'm grateful for that.
Fabiano Furtado
More information about the nginx-devel
mailing list