Honor (MIME) `"types"` in `"index"`

nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.

https://unit.nginx.org

Apache License 2.0

5.27k stars 322 forks source link

Honor (MIME) `"types"` in `"index"` #1080

Closed alejandro-colomar closed 5 months ago

alejandro-colomar commented 5 months ago

Reported-by: @mqudsi Closes: https://github.com/nginx/unit/issues/1079

@ac000 I have not tested this code at all (that's above my paygrade). I've fixed this since I wrote part of that code.

mqudsi commented 5 months ago

So what happes if the index doesn't match? It moves on to the next rule, but does it use the old path or the new path from the index (if index exists)? Ideally the latter, I think?

alejandro-colomar commented 5 months ago

So what happes if the index doesn't match? It moves on to the next rule, but does it use the old path or the new path from the index (if index exists)? Ideally the latter, I think?

The same exact thing as if the index file didn't exist, IIRC.

If you specified several possible index files, it will try the next one. Otherwise, if you specified several possible share files, it will try the next one. Otherwise, the entire action will fail, and if there's a fallback, the fallback action will be attempted (the fallback action knows nothing about the initial action, so it doesn't try the same index).

I'm talking from memory, since I didn't look at that code in a year or so.

alejandro-colomar commented 5 months ago

v2 changes:

Blame the introduction of the bug

$ git range-diff master gh/mtypes mtypes 
1:  3fd3bbf3 = 1:  3fd3bbf3 HTTP: static: refactor: Use shr instead of ctx->share
2:  b42ac153 = 2:  b42ac153 HTTP: static: refactor: Use a common variable in both index and non-index code
3:  0590d5dc = 3:  0590d5dc HTTP: static: refactor: Split conditional
4:  bbcf51f8 = 4:  bbcf51f8 HTTP: static: refactor: Use path instead of shr
5:  0e22e6ed = 5:  0e22e6ed HTTP: static: refactor: Deduplicate code
6:  026439ea = 6:  026439ea HTTP: static: refactor: Remove dead code
7:  f58f0d13 ! 7:  9e3c7b20 HTTP: static: Honor (MIME) "types" in "index"
    @@ Commit message
         Otherwise, we might end up serving some file that we didn't want to
         serve.

    +    Fixes: c5220944d2ac ("Static: variables in the "share" option.")
         Closes: <https://github.com/nginx/unit/issues/1079>
         Reported-by: Mahmoud Al-Qudsi <https://github.com/mqudsi>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

It seems that the bug was introduced in c5220944d2ac (@hongzhidao).

alejandro-colomar commented 5 months ago

v2b changes:

Cc @hongzhidao

$ git range-diff master gh/mtypes mtypes 
1:  3fd3bbf3 = 1:  3fd3bbf3 HTTP: static: refactor: Use shr instead of ctx->share
2:  b42ac153 = 2:  b42ac153 HTTP: static: refactor: Use a common variable in both index and non-index code
3:  0590d5dc = 3:  0590d5dc HTTP: static: refactor: Split conditional
4:  bbcf51f8 = 4:  bbcf51f8 HTTP: static: refactor: Use path instead of shr
5:  0e22e6ed = 5:  0e22e6ed HTTP: static: refactor: Deduplicate code
6:  026439ea = 6:  026439ea HTTP: static: refactor: Remove dead code
7:  9e3c7b20 ! 7:  bce69d2d HTTP: static: Honor (MIME) "types" in "index"
    @@ Commit message
         Fixes: c5220944d2ac ("Static: variables in the "share" option.")
         Closes: <https://github.com/nginx/unit/issues/1079>
         Reported-by: Mahmoud Al-Qudsi <https://github.com/mqudsi>
    +    Cc: Zhidao HONG <z.hong@f5.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##

tippexs commented 5 months ago

Thanks @alejandro-colomar for this PR! Good to see you back in action! I have commented on the issue with my test result and we will have a look on the Code and provide feedback. Please note that our contribution guidelines outline that a PR MUST BE fully tested before submitted. https://github.com/nginx/unit/blob/master/CONTRIBUTING.md#open-a-pull-request

Sorry, if that wasn't clear enough and I will make sure we will add some more information to the docs what "tested" exactly means. Will be good for this time :) Just a reminder for the -hopefully- next PR.

callahad commented 5 months ago

Please not that our contribution guidelines outline that a PR MUST BE fully tested before submitted.

I'd like to gently walk this back: Alex is contributing as an external volunteer. While prior testing is appreciated, it's up to us to test and validate proposed changes before inclusion.

alejandro-colomar commented 5 months ago

Thanks @alejandro-colomar for this PR! Good to see you back in action! I have commented on the issue with my test result and we will have a look on the Code and provide feedback. Please not that our contribution guidelines outline that a PR MUST BE fully tested before submitted. https://github.com/nginx/unit/blob/master/CONTRIBUTING.md#open-a-pull-request

Sorry, if that wasn't clear enough and I will make sure we will add some more information to the docs what "tested" exactly means. Will be good for this time :) Just a reminder for the -hopefully- next PR.

I didn't even look at that file, so don't worry, it's probably clear enough. And yeah, I usually test my patches before contributing to open-source projects. However, I think it wouldn't be fair to give F5 all this work for free, after all the events that we both know.

I give you these patches, because I had fun writing them, but you'll have to test them yourself if you want them, or pay me if you want me to test them. As a Spanish artist once said: "I sell you my time, I give you my art".

alejandro-colomar commented 5 months ago

v2c changes:

tfix

$ git range-diff master gh/mtypes mtypes 
1:  3fd3bbf3 = 1:  3fd3bbf3 HTTP: static: refactor: Use shr instead of ctx->share
2:  b42ac153 = 2:  b42ac153 HTTP: static: refactor: Use a common variable in both index and non-index code
3:  0590d5dc = 3:  0590d5dc HTTP: static: refactor: Split conditional
4:  bbcf51f8 = 4:  bbcf51f8 HTTP: static: refactor: Use path instead of shr
5:  0e22e6ed = 5:  0e22e6ed HTTP: static: refactor: Deduplicate code
6:  026439ea ! 6:  9e398046 HTTP: static: refactor: Remove dead code
    @@ Metadata
      ## Commit message ##
         HTTP: static: refactor: Remove dead code

    -    Don't set exten to NULL, just to reset it later... to the same exact
    +    Don't set extern to NULL, just to reset it later... to the same exact
         thing.

         Signed-off-by: Alejandro Colomar <alx@kernel.org>
7:  bce69d2d = 7:  ee7d373d HTTP: static: Honor (MIME) "types" in "index"

tippexs commented 5 months ago

Thanks @alejandro-colomar for this PR! Good to see you back in action! I have commented on the issue with my test result and we will have a look on the Code and provide feedback. Please not that our contribution guidelines outline that a PR MUST BE fully tested before submitted. https://github.com/nginx/unit/blob/master/CONTRIBUTING.md#open-a-pull-request Sorry, if that wasn't clear enough and I will make sure we will add some more information to the docs what "tested" exactly means. Will be good for this time :) Just a reminder for the -hopefully- next PR.

I didn't even look at that file, so don't worry, it's probably clear enough. And yeah, I usually test my patches before contributing to open-source projects. However, I think it wouldn't be fair to give F5 all this work for free, after all the events that we both know.

I give you these patches, because I had fun writing them, but you'll have to test them yourself if you want them, or pay me if you want me to test them. As a Spanish artist once said: "I sell you my time, I give you my art".

I know @alejandro-colomar :) It was not ment to be a super hard requirement but I wanted to point that out for further documentation. There is definitely room for improvement on our side as well. It is great seeing you back and I am already on it testing the patch. As already mentioned - great to see you back and I appreciate your contribution and time!

ac000 commented 5 months ago

Hi Alex!,

$ git range-diff master gh/mtypes mtypes 
...
6:  026439ea ! 6:  9e398046 HTTP: static: refactor: Remove dead code
    @@ Metadata
      ## Commit message ##
         HTTP: static: refactor: Remove dead code

    -    Don't set exten to NULL, just to reset it later... to the same exact
    +    Don't set extern to NULL, just to reset it later... to the same exact
         thing.

You had it (exten) right the first time ;).

alejandro-colomar commented 5 months ago

Hey Andrew!

Heh! ROFL. Thanks for catching the antitypo! :D

I'll fix it in a moment. For now,

v3 changes:

Collapse two commits into one. They are still meaningful as a single commit.

$ git range-diff master gh/mtypes mtypes 
1:  3fd3bbf3 = 1:  3fd3bbf3 HTTP: static: refactor: Use shr instead of ctx->share
2:  b42ac153 = 2:  b42ac153 HTTP: static: refactor: Use a common variable in both index and non-index code
3:  0590d5dc = 3:  0590d5dc HTTP: static: refactor: Split conditional
4:  bbcf51f8 < -:  -------- HTTP: static: refactor: Use path instead of shr
5:  0e22e6ed ! 4:  cfa036ac HTTP: static: refactor: Deduplicate code
    @@ src/nxt_http_static.c: nxt_http_static_send_ready(nxt_task_t *task, void *obj, v
                  nxt_str_null(&exten);

              } else {
    --            nxt_http_static_extract_extension(path, &exten);
    +-            nxt_http_static_extract_extension(shr, &exten);
                  mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

                  ret = nxt_http_route_test_rule(r, conf->types, mtype->start,
6:  9e398046 = 5:  5686c87f HTTP: static: refactor: Remove dead code
7:  ee7d373d = 6:  787fe720 HTTP: static: Honor (MIME) "types" in "index"

Edited: it seems my clipboard didn't have the right thing. :)

alejandro-colomar commented 5 months ago

And v3b changes:

Unfix antitypo in commit message. [@ac000 ]

$ git range-diff master gh/mtypes mtypes 
1:  3fd3bbf3 = 1:  3fd3bbf3 HTTP: static: refactor: Use shr instead of ctx->share
2:  b42ac153 = 2:  b42ac153 HTTP: static: refactor: Use a common variable in both index and non-index code
3:  0590d5dc = 3:  0590d5dc HTTP: static: refactor: Split conditional
4:  cfa036ac = 4:  cfa036ac HTTP: static: refactor: Deduplicate code
5:  5686c87f ! 5:  7c4020e2 HTTP: static: refactor: Remove dead code
    @@ Metadata
      ## Commit message ##
         HTTP: static: refactor: Remove dead code

    -    Don't set extern to NULL, just to reset it later... to the same exact
    +    Don't set exten to NULL, just to reset it later... to the same exact
         thing.

    +    Cc: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
6:  787fe720 = 6:  d23120cc HTTP: static: Honor (MIME) "types" in "index"

alejandro-colomar commented 5 months ago

v3c changes:

It is not a regression. It was introduced in the initial implementation of the feature with that behavior already. It was just that the sneaky refactor looked very suspicious, but it was a false positive.

$ git range-diff master gh/mtypes mtypes 
1:  3fd3bbf3 = 1:  3fd3bbf3 HTTP: static: refactor: Use shr instead of ctx->share
2:  b42ac153 = 2:  b42ac153 HTTP: static: refactor: Use a common variable in both index and non-index code
3:  0590d5dc = 3:  0590d5dc HTTP: static: refactor: Split conditional
4:  cfa036ac = 4:  cfa036ac HTTP: static: refactor: Deduplicate code
5:  7c4020e2 = 5:  7c4020e2 HTTP: static: refactor: Remove dead code
6:  d23120cc ! 6:  4ac0b6ad HTTP: static: Honor (MIME) "types" in "index"
    @@ Commit message
         Otherwise, we might end up serving some file that we didn't want to
         serve.

    -    Fixes: c5220944d2ac ("Static: variables in the "share" option.")
    +    Fixes: b9d5eb285a2f ("Static: implemented MIME filtering")
         Closes: <https://github.com/nginx/unit/issues/1079>
         Reported-by: Mahmoud Al-Qudsi <https://github.com/mqudsi>
         Cc: Zhidao HONG <z.hong@f5.com>
    +    Cc: Oisin Canty <https://github.com/ocanty>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##

ac000 commented 5 months ago

I'm not sure we need/want 'refactor' in the commit subjects. It'll also make an overly long subject line, tolerable...

I think the commit subjects are already self-explanatory without it.

alejandro-colomar commented 5 months ago

I'm not sure we need/want 'refactor' in the commit subjects. It'll also make an overly long subject line, tolerable...

I think the commit subjects are already self-explanatory without it.

Yup. I started using those in the gzip patch series, when Z said he wanted to have a clear indication of what patches don't modify behavior. I prefer without them, per the reasons you say. I'll drop them.

alejandro-colomar commented 5 months ago

I've moved them to inside the commit message.

v3d changes:

tweak commit messages [@ac000 ]

$ git range-diff master gh/mtypes mtypes 
1:  3fd3bbf3 ! 1:  ec39c37a HTTP: static: refactor: Use shr instead of ctx->share
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: refactor: Use shr instead of ctx->share
    +    HTTP: static: Use shr instead of ctx->share
    +
    +    Refactor.

         They are the same thing.  This is in preparation for the next commits.

    +    Cc: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
2:  b42ac153 ! 2:  cc01caef HTTP: static: refactor: Use a common variable in both index and non-index code
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: refactor: Use a common variable in both index and non-index code
    +    HTTP: static: Use a common variable in both index and non-index code
    +
    +    Refactor.

         Use a common variable, path, that will hold the entire path of the file.
         This is in preparation for the following commits.

         Also, add myself to copyright.

    +    Cc: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
3:  0590d5dc ! 3:  a2fc2fab HTTP: static: refactor: Split conditional
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: refactor: Split conditional
    +    HTTP: static: Split conditional
    +
    +    Refactor.

         Separate a conditional into two blocks, and reverse the logic in the
         first one.  This is in preparation for the following commits.

    +    Cc: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
4:  cfa036ac ! 4:  c72bb7b2 HTTP: static: refactor: Deduplicate code
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: refactor: Deduplicate code
    +    HTTP: static: Deduplicate code

    +    Refactor.
    +
    +    Cc: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
5:  7c4020e2 ! 5:  05943460 HTTP: static: refactor: Remove dead code
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: refactor: Remove dead code
    +    HTTP: static: Remove dead code
    +
    +    Refactor.

         Don't set exten to NULL, just to reset it later... to the same exact
         thing.
6:  4ac0b6ad ! 6:  9c0c2d93 HTTP: static: Honor (MIME) "types" in "index"
    @@ Commit message
         Reported-by: Mahmoud Al-Qudsi <https://github.com/mqudsi>
         Cc: Zhidao HONG <z.hong@f5.com>
         Cc: Oisin Canty <https://github.com/ocanty>
    +    Cc: Andrew Clayton <a.clayton@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##

alejandro-colomar commented 5 months ago

@ac000

After
05943460 ("HTTP: static: refactor: Remove dead code")
exten is only used in the second if block. It could be localised to there...

It's used later again:

$ grepc -tfd nxt_http_static_send_ready . | grep -n exten
9:    nxt_str_t               *shr, *index, *path, exten, *mtype;
51:    nxt_http_static_extract_extension(path, &exten);
54:        mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);
253:            mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

You can see exten colored in that function with the code below (-r for color) (but not the color that github shows, it's a grep-like color):

$ grepc -htfd nxt_http_static_send_ready . | grepc -rtu exten
(standard input):static void
nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
{
    size_t                  length, encode;
    u_char                  *p, *fname;
    struct tm               tm;
    nxt_buf_t               *fb;
    nxt_int_t               ret;
    nxt_str_t               *shr, *index, *path, exten, *mtype;
    nxt_uint_t              level;
    nxt_file_t              *f, file;
    nxt_file_info_t         fi;
    nxt_http_field_t        *field;
    nxt_http_status_t       status;
    nxt_router_conf_t       *rtcf;
    nxt_http_action_t       *action;
    nxt_http_request_t      *r;
    nxt_work_handler_t      body_handler;
    nxt_http_static_ctx_t   *ctx;
    nxt_http_static_conf_t  *conf;

    ...

    nxt_http_static_extract_extension(path, &exten);

    if (conf->types != NULL) {
        mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

        ret = nxt_http_route_test_rule(r, conf->types, mtype->start,
                                       mtype->length);
        if (nxt_slow_path(ret == NXT_ERROR)) {
            goto fail;
        }

        if (ret == 0) {
            nxt_http_static_next(task, r, ctx, NXT_HTTP_FORBIDDEN);
            return;
        }
    }

    ...

    if (nxt_fast_path(nxt_is_file(&fi))) {
        ...

        if (mtype == NULL) {
            mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);
        }

        ...

    } else {
        ...
    }

    ...
}

Although, maybe I can also join those two into an unconditional

mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

Here's all that happens to mtype:

$ grepc -tfd nxt_http_static_send_ready . | grep -n mtype
9:    nxt_str_t               *shr, *index, *path, exten, *mtype;
29:    mtype = NULL;
54:        mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);
56:        ret = nxt_http_route_test_rule(r, conf->types, mtype->start,
57:                                       mtype->length);
252:        if (mtype == NULL) {
253:            mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);
256:        if (mtype->length != 0) {
264:            field->value = mtype->start;
265:            field->value_length = mtype->length;

It doesn't look like a heavy call, since it works with a short suffix string, and then checks a table to see if the extension is a known mime type, so it may be worth running unconditionally. But, that's probably worth a separate PR. And still, since it doesn't reduce much code, it may not be worth it.

alejandro-colomar commented 5 months ago

Oh well, that rabbit hole wasn't shallow. I found that the original call to

nxt_http_static_extract_extension(index, &exten);

was completely gratuituous. It was dead code right from the beginning. (To be precise, it wasn't dead code, because nobody thought of forming a nxt_str_t joining shr and index, so it wasn't possible to extract the extension in a different way, but yeah, conceptually, dead code).

$ git show c5220944d2ac^:src/nxt_http_static.c | grepc nxt_http_static_send_ready | grep exten -C1
    nxt_int_t               ret;
    nxt_str_t               index, exten, *mtype;
    nxt_uint_t              level;
--
        nxt_str_set(&index, "index.html");
        nxt_str_set(&exten, ".html");

--
        nxt_str_set(&index, "");
        nxt_str_null(&exten);
    }
--

    if (conf->types != NULL && exten.start == NULL) {
        nxt_http_static_extract_extension(r->path, &exten);
        mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

--

        if (exten.start == NULL) {
            nxt_http_static_extract_extension(r->path, &exten);
        }
--
        if (mtype == NULL) {
            mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);
        }

Then Z's refactor made it even more obvious that it was dead code.

$ git show c5220944d2ac:src/nxt_http_static.c | grepc nxt_http_static_send_ready | grep exten -C1
    nxt_int_t               ret;
    nxt_str_t               *shr, exten, *mtype;
    nxt_uint_t              level;
--
        /* TODO: dynamic index setting. */
        nxt_str_set(&exten, ".html");

--
        if (conf->types == NULL) {
            nxt_str_null(&exten);

        } else {
            nxt_http_static_extract_extension(shr, &exten);
            mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

--

        if (exten.start == NULL) {
            nxt_http_static_extract_extension(shr, &exten);
        }
--
        if (mtype == NULL) {
            mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);
        }

If they were happy paying the cost of setting the extension unconditionally so early (if path[-1] == '/', which is a pretty common case), we should be happy paying the cost of calling nxt_http_static_mtype_get() early too.

alejandro-colomar commented 5 months ago

v4 changes:

Deduplicate code [@ac000 ]

$ git range-diff master gh/mtypes mtypes 
1:  ec39c37a = 1:  ec39c37a HTTP: static: Use shr instead of ctx->share
2:  cc01caef = 2:  cc01caef HTTP: static: Use a common variable in both index and non-index code
3:  a2fc2fab = 3:  a2fc2fab HTTP: static: Split conditional
4:  c72bb7b2 = 4:  c72bb7b2 HTTP: static: Deduplicate code
5:  05943460 = 5:  05943460 HTTP: static: Remove dead code
6:  9c0c2d93 = 6:  9c0c2d93 HTTP: static: Honor (MIME) "types" in "index"
-:  -------- > 7:  1ffa5fb2 HTTP: static: Deduplicate code

alejandro-colomar commented 5 months ago

And another rabbit hole. I bet this rabbit is of the security subspecies.

Below is a diff of nxt_http_static_send_ready() by the commit c5220944d2ac.

$ diff -u <(git show c5220944d2ac^:src/nxt_http_static.c | grepc nxt_http_static_send_ready) <(git show c5220944d2ac:src/nxt_http_static.c | grepc nxt_http_static_send_ready)
--- /dev/fd/63  2024-01-23 14:23:18.419571420 +0100
+++ /dev/fd/62  2024-01-23 14:23:18.419571420 +0100
@@ -7,7 +7,7 @@
     struct tm               tm;
     nxt_buf_t               *fb;
     nxt_int_t               ret;
-    nxt_str_t               index, exten, *mtype;
+    nxt_str_t               *shr, exten, *mtype;
     nxt_uint_t              level;
     nxt_file_t              *f, file;
     nxt_file_info_t         fi;
@@ -20,56 +20,58 @@
     nxt_http_static_ctx_t   *ctx;
     nxt_http_static_conf_t  *conf;

+    static nxt_str_t  index = nxt_string("index.html");
+
     r = obj;
     ctx = data;
     action = ctx->action;
     conf = action->u.conf;
-
-    if (r->path->start[r->path->length - 1] == '/') {
-        /* TODO: dynamic index setting. */
-        nxt_str_set(&index, "index.html");
-        nxt_str_set(&exten, ".html");
-
-    } else {
-        nxt_str_set(&index, "");
-        nxt_str_null(&exten);
-    }
+    rtcf = r->conf->socket_conf->router_conf;

     f = NULL;
+    mtype = NULL;
     status = NXT_HTTP_INTERNAL_SERVER_ERROR;

-    rtcf = r->conf->socket_conf->router_conf;
+    shr = &ctx->share;

-    mtype = NULL;
+    if (shr->start[shr->length - 1] == '/') {
+        /* TODO: dynamic index setting. */
+        nxt_str_set(&exten, ".html");

-    if (conf->types != NULL && exten.start == NULL) {
-        nxt_http_static_extract_extension(r->path, &exten);
-        mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);
+        length = shr->length + index.length;

-        ret = nxt_http_route_test_rule(r, conf->types, mtype->start,
-                                       mtype->length);
-        if (nxt_slow_path(ret == NXT_ERROR)) {
+        fname = nxt_mp_nget(r->mem_pool, length + 1);
+        if (nxt_slow_path(fname == NULL)) {
             goto fail;
         }

-        if (ret == 0) {
-            status = NXT_HTTP_FORBIDDEN;
-            goto fail;
-        }
-    }
+        p = fname;
+        p = nxt_cpymem(p, shr->start, shr->length);
+        p = nxt_cpymem(p, index.start, index.length);
+        *p = '\0';

-    length = conf->share.length + r->path->length + index.length;
+    } else {
+        if (conf->types == NULL) {
+            nxt_str_null(&exten);

-    fname = nxt_mp_nget(r->mem_pool, length + 1);
-    if (nxt_slow_path(fname == NULL)) {
-        goto fail;
-    }
+        } else {
+            nxt_http_static_extract_extension(shr, &exten);
+            mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

-    p = fname;
-    p = nxt_cpymem(p, conf->share.start, conf->share.length);
-    p = nxt_cpymem(p, r->path->start, r->path->length);
-    p = nxt_cpymem(p, index.start, index.length);
-    *p = '\0';
+            ret = nxt_http_route_test_rule(r, conf->types, mtype->start,
+                                           mtype->length);
+            if (nxt_slow_path(ret == NXT_ERROR)) {
+                goto fail;
+            }
+
+            if (ret == 0) {
+                status = NXT_HTTP_FORBIDDEN;
+                goto fail;
+            }
+        }
+
+        fname = ctx->share.start;
+    }

     nxt_memzero(&file, sizeof(nxt_file_t));

@@ -86,7 +88,9 @@
         if (chr->length > 0) {
             resolve |= RESOLVE_IN_ROOT;

-            fname = nxt_http_static_chroot_match(chr->start, file.name);
+            fname = conf->is_const
+                    ? conf->fname
+                    : nxt_http_static_chroot_match(chr->start, file.name);

             if (fname != NULL) {
                 file.name = chr->start;
@@ -247,7 +251,7 @@
                               - p;

         if (exten.start == NULL) {
-            nxt_http_static_extract_extension(r->path, &exten);
+            nxt_http_static_extract_extension(shr, &exten);
         }

         if (mtype == NULL) {

It's quite unreadable (thanks for embedding a refactor there), so let's see the code before and after.

After:

$ git show c5220944d2ac:src/nxt_http_static.c | grepc -tfd nxt_http_static_send_ready
(standard input):static void
nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
{
    ...

    shr = &ctx->share;

    if (shr->start[shr->length - 1] == '/') {
        /* TODO: dynamic index setting. */
        nxt_str_set(&exten, ".html");

        length = shr->length + index.length;

        fname = nxt_mp_nget(r->mem_pool, length + 1);
        if (nxt_slow_path(fname == NULL)) {
            goto fail;
        }

        p = fname;
        p = nxt_cpymem(p, shr->start, shr->length);
        p = nxt_cpymem(p, index.start, index.length);
        *p = '\0';

    } else {
        ...

        fname = ctx->share.start;
    }

    nxt_memzero(&file, sizeof(nxt_file_t));

    file.name = fname;

...
}

See where fname comes from? If we're using the index, it is shr + index + '\0'. But if we're not using it, it is just shr (yes, shr, not shr + '\0').

Now, let's check the code before.

$ git show c5220944d2ac^:src/nxt_http_static.c | grepc -tfd nxt_http_static_send_ready
(standard input):static void
nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
{
    ...

    if (r->path->start[r->path->length - 1] == '/') {
        /* TODO: dynamic index setting. */
        nxt_str_set(&index, "index.html");
        nxt_str_set(&exten, ".html");

    } else {
        nxt_str_set(&index, "");
        nxt_str_null(&exten);
    }

    ...

    length = conf->share.length + r->path->length + index.length;

    fname = nxt_mp_nget(r->mem_pool, length + 1);
    if (nxt_slow_path(fname == NULL)) {
        goto fail;
    }

    p = fname;
    p = nxt_cpymem(p, conf->share.start, conf->share.length);
    p = nxt_cpymem(p, r->path->start, r->path->length);
    p = nxt_cpymem(p, index.start, index.length);
    *p = '\0';

    nxt_memzero(&file, sizeof(nxt_file_t));

    file.name = fname;

...
}

Heh, the original code was shr [+ index] + '\0'. So the introduction of that problem was c5220944d2ac.

Now, why didn't that open hells doors before? I don't know. I didn't dig that far.

alejandro-colomar commented 5 months ago

v5 changes:

Fix several important bugs introduced in c5220944d2ac.

The range-diff is horrendous; I won't show it.

Here's the oneline log:

$ git log --oneline master..mtypes 
c05b1eca (HEAD -> mtypes, gh/mtypes, types) HTTP: static: Deduplicate code
89eb3ef2 HTTP: static: Honor (MIME) "types" in "index"
b8f19275 HTTP: static: Terminate string with a null byte
21645319 HTTP: static: Remove dead code
67d7074d HTTP: static: Deduplicate code
59d39195 HTTP: static: Don't include the terminating null byte in exten
ce01dfc4 HTTP: static: Use path instead of shr for extracting the extension
a2fc2fab HTTP: static: Split conditional
cc01caef HTTP: static: Use a common variable in both index and non-index code
ec39c37a HTTP: static: Use shr instead of ctx->share

And here's the interdiff:

$ git diff 1ffa5fb
diff --git a/src/nxt_http_static.c b/src/nxt_http_static.c
index ab5af994..90a13cb5 100644
--- a/src/nxt_http_static.c
+++ b/src/nxt_http_static.c
@@ -334,23 +334,23 @@ nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
     shr = &ctx->share;
     index = &conf->index;

-    if (shr->start[shr->length - 1] != '/') {
-        path = shr;
-
-    } else {
-        length = shr->length + index->length;
-
-        path = nxt_str_alloc(r->mem_pool, length + 1);
-        if (nxt_slow_path(path == NULL)) {
-            goto fail;
-        }
-
-        p = path->start;
-        p = nxt_cpymem(p, shr->start, shr->length);
-        p = nxt_cpymem(p, index->start, index->length);
-        *p = '\0';
+    length = shr->length + 1;
+    if (shr->start[shr->length - 1] == '/') {
+        length += index->length;
     }

+    path = nxt_str_alloc(r->mem_pool, length + 1);
+    if (nxt_slow_path(path == NULL)) {
+        goto fail;
+    }
+
+    p = path->start;
+    p = nxt_cpymem(p, shr->start, shr->length);
+    if (shr->start[shr->length - 1] == '/') {
+        p = nxt_cpymem(p, index->start, index->length);
+    }
+    *p = '\0';
+
     nxt_http_static_extract_extension(path, &exten);
     mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

@@ -776,7 +776,7 @@ nxt_http_static_extract_extension(nxt_str_t *path, nxt_str_t *exten)

 extension:

-    exten->length = end - p;
+    exten->length = end - p - 1;
     exten->start = p;
 }

ac000 commented 5 months ago

exten is only used in the second if block. It could be localised to there...

It's used later again:

$ grepc -tfd nxt_http_static_send_ready . | grep -n exten
9:    nxt_str_t               *shr, *index, *path, exten, *mtype;
51:    nxt_http_static_extract_extension(path, &exten);
54:        mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);
253:            mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

Hmm, for some reason (function already seemed long enough at that point) I stopped reading after the #if (NXT_HAVE_OPENAT2)

alejandro-colomar commented 5 months ago

v5b changes:

wfix commit message

alejandro-colomar commented 5 months ago

v5c changes:

I forgot to remove the +1 when I put it above, and it resulted in a +2. Fixed.

$ git range-diff master gh/mtypes mtypes 
 1:  ec39c37a =  1:  ec39c37a HTTP: static: Use shr instead of ctx->share
 2:  cc01caef =  2:  cc01caef HTTP: static: Use a common variable in both index and non-index code
 3:  a2fc2fab =  3:  a2fc2fab HTTP: static: Split conditional
 4:  ce01dfc4 =  4:  ce01dfc4 HTTP: static: Use path instead of shr for extracting the extension
 5:  57899564 =  5:  57899564 HTTP: static: Don't include the terminating null byte in exten
 6:  104433e5 =  6:  104433e5 HTTP: static: Deduplicate code
 7:  ad56ec75 =  7:  ad56ec75 HTTP: static: Remove dead code
 8:  e83614d7 !  8:  9a8e6d92 HTTP: static: Terminate string with a null byte
    @@ src/nxt_http_static.c: nxt_http_static_send_ready(nxt_task_t *task, void *obj, v
     +        length += index->length;
          }

    -+    path = nxt_str_alloc(r->mem_pool, length + 1);
    ++    path = nxt_str_alloc(r->mem_pool, length);
     +    if (nxt_slow_path(path == NULL)) {
     +        goto fail;
     +    }
 9:  4df7b491 =  9:  e87a410d HTTP: static: Honor (MIME) "types" in "index"
10:  801d5b8c = 10:  0d2bd706 HTTP: static: Deduplicate code

alejandro-colomar commented 5 months ago

v5d changes:

Update pytest

$ git range-diff master gh/mtypes mtypes 
 1:  ec39c37a =  1:  ec39c37a HTTP: static: Use shr instead of ctx->share
 2:  cc01caef =  2:  cc01caef HTTP: static: Use a common variable in both index and non-index code
 3:  a2fc2fab =  3:  a2fc2fab HTTP: static: Split conditional
 4:  ce01dfc4 =  4:  ce01dfc4 HTTP: static: Use path instead of shr for extracting the extension
 5:  57899564 =  5:  57899564 HTTP: static: Don't include the terminating null byte in exten
 6:  104433e5 =  6:  104433e5 HTTP: static: Deduplicate code
 7:  ad56ec75 =  7:  ad56ec75 HTTP: static: Remove dead code
 8:  9a8e6d92 =  8:  9a8e6d92 HTTP: static: Terminate string with a null byte
 9:  e87a410d !  9:  3d567775 HTTP: static: Honor (MIME) "types" in "index"
    @@ Commit message
         Otherwise, we might end up serving some file that we didn't want to
         serve.

    +    Adapt the corresponding test.
    +
         Fixes: b9d5eb285a2f ("Static: implemented MIME filtering")
         Closes: <https://github.com/nginx/unit/issues/1079>
         Reported-by: Mahmoud Al-Qudsi <https://github.com/mqudsi>
    @@ src/nxt_http_static.c: nxt_http_static_send_ready(nxt_task_t *task, void *obj, v
              }
          }

    +
    + ## test/test_static_types.py ##
    +@@ test/test_static_types.py: def test_static_types_index(temp_dir):
    +     action_update(
    +         {"share": f'{temp_dir}/assets$uri', "types": "application/xml"}
    +     )
    +-    check_body('/', 'index')
    +     check_body('/file.xml', '.xml')
    ++    assert client.get(url='/')['status'] == 403, 'forbidden mtype'
    +     assert client.get(url='/index.html')['status'] == 403, 'forbidden mtype'
    +     assert client.get(url='/file.mp4')['status'] == 403, 'forbidden mtype'
    + 
10:  0d2bd706 = 10:  7feceaf3 HTTP: static: Deduplicate code

(Edited: again, wrong clipboard.)

alejandro-colomar commented 5 months ago

v5e changes:

Be more precise in the copyright years. [@callahad ]

$ git range-diff master gh/mtypes mtypes 
 1:  ec39c37a =  1:  ec39c37a HTTP: static: Use shr instead of ctx->share
 2:  cc01caef !  2:  75257300 HTTP: static: Use a common variable in both index and non-index code
    @@ src/nxt_http_static.c

      /*
       * Copyright (C) NGINX, Inc.
    -+ * Copyright 2021-2024, Alejandro Colomar <alx@kernel.org>
    ++ * Copyright 2021-2022, 2024, Alejandro Colomar <alx@kernel.org>
       */

      #include <nxt_router.h>
 3:  a2fc2fab =  3:  454cc926 HTTP: static: Split conditional
 4:  ce01dfc4 =  4:  f5f9caa6 HTTP: static: Use path instead of shr for extracting the extension
 5:  57899564 =  5:  d4c1bb1c HTTP: static: Don't include the terminating null byte in exten
 6:  104433e5 =  6:  ee1912d8 HTTP: static: Deduplicate code
 7:  ad56ec75 =  7:  e2f67946 HTTP: static: Remove dead code
 8:  9a8e6d92 =  8:  e25c80ea HTTP: static: Terminate string with a null byte
 9:  3d567775 =  9:  4048c67e HTTP: static: Honor (MIME) "types" in "index"
10:  7feceaf3 = 10:  47c5f1e9 HTTP: static: Deduplicate code

alejandro-colomar commented 5 months ago

(I posted a range-diff for v5e, but forgot to push the changes; they're uploaded now)

andrey-zelenkov commented 5 months ago

HTTP: static: We are not using more than one prefix; the most specific is enough.

The first four commits don't make sense without the commit where they are actually needed, and the state of the repo between them looks strange. Please collapse them. Nobody should suffer, like Sheldon from the meme, untangling this history.

HTTP: static: Remove dead code Dead code is the code that is never executed, so the message is misleading and should be fixed.

HTTP: static: Deduplicate code Duplication of the code was introduced in the previous commit. There's no need to commit a mistake and then fix it in the next commit if you can simply avoid making a mistake.

alejandro-colomar commented 5 months ago

Hi Andrei!

HTTP: static: We are not using more than one prefix; the most specific is enough.

Ok. I'll use Static:

The first four commits don't make sense without the commit where they are actually needed, and the state of the repo between them looks strange. Please collapse them.

Do you mean collapsing 1-4, or 1-5?

Commit 5 is a bugfix. I certainly want to isolate that bugfix from any refactor (that's precisely the reason the bug was introduced in the first place).

Regarding collapsing 1-4, I think that's too much information for a single commit. The diff is a bit too complex to consider it trivial.

Regarding the state between commits:

1 (Use shr instead of ctx->share): This simplifies code. It is better afterwards.
2 (Use a common variable in both index and non-index code): This adds one variable, but moves another out of a conditional, so it's not that bad.
3 (Split conditional): Admitted; the state after this commit is a bit weird. But collapsing it with the next one makes it significantly more complex.
4 (Use path instead of shr for extracting the extension): This makes code simpler, reducing the uses of the shr variable to the bare minimum.

(Here's how it would look like:)

$ git diff f5f9caa6^^^^..f5f9caa6
diff --git a/src/nxt_http_static.c b/src/nxt_http_static.c
index c4caab3c..232761c1 100644
--- a/src/nxt_http_static.c
+++ b/src/nxt_http_static.c
@@ -1,6 +1,7 @@

 /*
  * Copyright (C) NGINX, Inc.
+ * Copyright 2021-2022, 2024, Alejandro Colomar <alx@kernel.org>
  */

 #include <nxt_router.h>
@@ -308,7 +309,7 @@ nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
     struct tm               tm;
     nxt_buf_t               *fb;
     nxt_int_t               ret;
-    nxt_str_t               *shr, *index, exten, *mtype;
+    nxt_str_t               *shr, *index, *path, exten, *mtype;
     nxt_uint_t              level;
     nxt_file_t              *f, file;
     nxt_file_info_t         fi;
@@ -333,27 +334,31 @@ nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
     shr = &ctx->share;
     index = &conf->index;

-    if (shr->start[shr->length - 1] == '/') {
+    if (shr->start[shr->length - 1] != '/') {
+        path = shr;
+
+    } else {
         nxt_http_static_extract_extension(index, &exten);

         length = shr->length + index->length;

-        fname = nxt_mp_nget(r->mem_pool, length + 1);
-        if (nxt_slow_path(fname == NULL)) {
+        path = nxt_str_alloc(r->mem_pool, length + 1);
+        if (nxt_slow_path(path == NULL)) {
             goto fail;
         }

-        p = fname;
+        p = path->start;
         p = nxt_cpymem(p, shr->start, shr->length);
         p = nxt_cpymem(p, index->start, index->length);
         *p = '\0';
+    }

-    } else {
+    if (shr->start[shr->length - 1] != '/') {
         if (conf->types == NULL) {
             nxt_str_null(&exten);

         } else {
-            nxt_http_static_extract_extension(shr, &exten);
+            nxt_http_static_extract_extension(path, &exten);
             mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

             ret = nxt_http_route_test_rule(r, conf->types, mtype->start,
@@ -367,10 +372,10 @@ nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
                 return;
             }
         }
-
-        fname = ctx->share.start;
     }

+    fname = path->start;
+
     nxt_memzero(&file, sizeof(nxt_file_t));

     file.name = fname;
@@ -554,7 +559,7 @@ nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
                               - p;

         if (exten.start == NULL) {
-            nxt_http_static_extract_extension(shr, &exten);
+            nxt_http_static_extract_extension(path, &exten);
         }

         if (mtype == NULL) {

Nobody should suffer, like Sheldon from the meme, untangling this history.

Maybe this PR should be merged with a merge commit. That would make clear that the entire patch set belongs to a whole set. Something like

*   0d52a558 (HEAD -> master) Merge branch 'mtypes'
|\  
| * 47c5f1e9 (gh/mtypes, mtypes) HTTP: static: Deduplicate code
| * 4048c67e HTTP: static: Honor (MIME) "types" in "index"
| * e25c80ea HTTP: static: Terminate string with a null byte
| * e2f67946 HTTP: static: Remove dead code
| * ee1912d8 HTTP: static: Deduplicate code
| * d4c1bb1c HTTP: static: Don't include the terminating null byte in exten
| * f5f9caa6 HTTP: static: Use path instead of shr for extracting the extension
| * 454cc926 HTTP: static: Split conditional
| * 75257300 HTTP: static: Use a common variable in both index and non-index code
| * ec39c37a HTTP: static: Use shr instead of ctx->share
|/  
* 02d1984c (ngx/master, gh/master) HTTP: Remove short read check in nxt_http_static_buf_completion()

HTTP: static: Remove dead code Dead code is the code that is never executed, so the message is misleading and should be fixed.

Unreachable code is code that is never executed. Dead code is a more general concept, which includes unreachable code, but also includes code that has no net effect, such as

x = 1;  // Dead code.
x = 2;

The compiler is allowed to remove dead code during compilation, and certainly, the compiler is allowed to remove the code that I removed in that commit. Admittedly, it was my fault, because I made it dead code in the previous commit, the one which extracts the extension unconditionally. That was the plan. If I collapse both commits, it gets messy:

$ git diff 46f8c01a^^..46f8c01a
diff --git a/src/nxt_http_static.c b/src/nxt_http_static.c
index 934a012c..8400fc04 100644
--- a/src/nxt_http_static.c
+++ b/src/nxt_http_static.c
@@ -349,16 +349,12 @@ nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
         p = nxt_cpymem(p, shr->start, shr->length);
         p = nxt_cpymem(p, index->start, index->length);
         *p = '\0';
-
-        nxt_http_static_extract_extension(path, &exten);
     }

-    if (shr->start[shr->length - 1] != '/') {
-        if (conf->types == NULL) {
-            nxt_str_null(&exten);
+    nxt_http_static_extract_extension(path, &exten);

-        } else {
-            nxt_http_static_extract_extension(path, &exten);
+    if (shr->start[shr->length - 1] != '/') {
+        if (conf->types != NULL) {
             mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

             ret = nxt_http_route_test_rule(r, conf->types, mtype->start,
@@ -558,10 +554,6 @@ nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
                                           nxt_file_size(&fi))
                               - p;

-        if (exten.start == NULL) {
-            nxt_http_static_extract_extension(path, &exten);
-        }
-
         if (mtype == NULL) {
             mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);
         }

Or maybe not. Now that I see that diff, it's reasonably clear.

HTTP: static: Deduplicate code Duplication of the code was introduced in the previous commit. There's no need to commit a mistake and then fix it in the next commit if you can simply avoid making a mistake.

I've been thinking, and a more appropriate commit message would be Unconditionally extract the extension. I wouldn't say it was a mistake, though; I did the previous commits precisely to allow doing this commit.

Thanks for the review! Cheers, Alex

alejandro-colomar commented 5 months ago

v6 changes:

Use Static:, not HTTP: static:. [@andrey-zelenkov ]
Collapse 6 and 7 [@andrey-zelenkov ]
Say Unconditionally extract the extension and Unconditionally get the MIME type instead of Deduplicate code It's more appropriate, since there's a small semantic change in the code.

$ git range-diff master gh/mtypes mtypes 
 1:  ec39c37a !  1:  6d2eb657 HTTP: static: Use shr instead of ctx->share
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: Use shr instead of ctx->share
    +    Static: Use shr instead of ctx->share

         Refactor.

         They are the same thing.  This is in preparation for the next commits.

         Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Cc: Andrei Zeliankou <zelenkov@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
 2:  75257300 !  2:  1a7048f8 HTTP: static: Use a common variable in both index and non-index code
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: Use a common variable in both index and non-index code
    +    Static: Use a common variable in both index and non-index code

         Refactor.

    @@ Commit message
         Also, add myself to copyright.

         Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Cc: Andrei Zeliankou <zelenkov@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
 3:  454cc926 !  3:  3a4c8a91 HTTP: static: Split conditional
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: Split conditional
    +    Static: Split conditional

         Refactor.

    @@ Commit message
         first one.  This is in preparation for the following commits.

         Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Cc: Andrei Zeliankou <zelenkov@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
 4:  f5f9caa6 !  4:  92d94903 HTTP: static: Use path instead of shr for extracting the extension
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: Use path instead of shr for extracting the extension
    +    Static: Use path instead of shr for extracting the extension

         Refactor.

         They point to the same string.  This is in preparation for the following
         commits.

    +    Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Cc: Andrei Zeliankou <zelenkov@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
 5:  d4c1bb1c !  5:  8fa7a02e HTTP: static: Don't include the terminating null byte in exten
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: Don't include the terminating null byte in exten
    +    Static: Don't include the terminating null byte in exten

         The 'path' is (or should be) terminated by a null byte ('\0').  But the
         code reading 'exten' assumes there's no null byte.  Thus, we must reduce
    @@ Commit message
         Fixes: c5220944d2ac ("Static: variables in the "share" option.")
         Cc: Andrew Clayton <a.clayton@nginx.com>
         Cc: Zhidao Hong <z.hong@f5.com>
    +    Cc: Andrei Zeliankou <zelenkov@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
 6:  ee1912d8 <  -:  -------- HTTP: static: Deduplicate code
 7:  e2f67946 !  6:  2fd35728 HTTP: static: Remove dead code
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: Remove dead code
    +    Static: Unconditionally extract the extension

         Refactor.

    -    Don't set exten to NULL, just to reset it later... to the same exact
    -    thing.
    -
         Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Cc: Andrei Zeliankou <zelenkov@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
     @@ src/nxt_http_static.c: nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
    -     nxt_http_static_extract_extension(path, &exten);
    +         p = nxt_cpymem(p, shr->start, shr->length);
    +         p = nxt_cpymem(p, index->start, index->length);
    +         *p = '\0';
    +-
    +-        nxt_http_static_extract_extension(path, &exten);
    +     }

    -     if (shr->start[shr->length - 1] != '/') {
    +-    if (shr->start[shr->length - 1] != '/') {
     -        if (conf->types == NULL) {
     -            nxt_str_null(&exten);
    --
    ++    nxt_http_static_extract_extension(path, &exten);
    + 
     -        } else {
    +-            nxt_http_static_extract_extension(path, &exten);
    ++    if (shr->start[shr->length - 1] != '/') {
     +        if (conf->types != NULL) {
                  mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);

 8:  e25c80ea !  7:  5b8b348d HTTP: static: Terminate string with a null byte
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: Terminate string with a null byte
    +    Static: Terminate string with a null byte

         This string is later passed to open(2).  It is also passed to
         nxt_http_static_extract_extension(), which also requires it to be
    @@ Commit message
         Fixes: 19a3dd75253b ("HTTP: static: Don't include NUL in exten")
         Cc: Andrew Clayton <a.clayton@nginx.com>
         Cc: Zhidao Hong <z.hong@f5.com>
    -    Signed-off-by: Alejandro Colomar <alx@kernel.org>
    +    Cc: Andrei Zeliankou <zelenkov@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
 9:  4048c67e !  8:  86f620d6 HTTP: static: Honor (MIME) "types" in "index"
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: Honor (MIME) "types" in "index"
    +    Static: Honor (MIME) "types" in "index"

         When we serve an index file, MIME type restrictions should still apply.
         Otherwise, we might end up serving some file that we didn't want to
    @@ Commit message
         Cc: Zhidao HONG <z.hong@f5.com>
         Cc: Oisin Canty <https://github.com/ocanty>
         Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Cc: Andrei Zeliankou <zelenkov@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##
10:  47c5f1e9 !  9:  a5a0beba HTTP: static: Deduplicate code
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    HTTP: static: Deduplicate code
    +    Static: Unconditionally get the MIME type

         Refactor.

         Cc: Andrew Clayton <a.clayton@nginx.com>
    +    Cc: Andrei Zeliankou <zelenkov@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## src/nxt_http_static.c ##

alejandro-colomar commented 5 months ago

v6b changes:

Small reword of commit message

$ git range-diff master gh/mtypes mtypes 
 1:  6d2eb657 =  1:  6d2eb657 Static: Use shr instead of ctx->share
 2:  1a7048f8 =  2:  1a7048f8 Static: Use a common variable in both index and non-index code
 3:  3a4c8a91 =  3:  3a4c8a91 Static: Split conditional
 4:  92d94903 =  4:  92d94903 Static: Use path instead of shr for extracting the extension
 5:  8fa7a02e =  5:  8fa7a02e Static: Don't include the terminating null byte in exten
 6:  2fd35728 =  6:  2fd35728 Static: Unconditionally extract the extension
 7:  5b8b348d !  7:  c8c4c70a Static: Terminate string with a null byte
    @@ Commit message
         terminated.

         This bug was introduced in c5220944d2ac, but for some reason was not
    -    discovered until recently, when I triggered it purposefully in
    -    19a3dd75253b.
    +    discovered until recently, when I triggered it purposefully two commits
    +    ago, while partially fixing the bug (or actually fixing another related
    +    bug, which was introduced in the same commit).

         Fixes: c5220944d2ac ("Static: variables in the "share" option.")
    -    Fixes: 19a3dd75253b ("HTTP: static: Don't include NUL in exten")
    +    Fixes: Two commits ago ("Static: Don't include the terminating null byte in exten")
         Cc: Andrew Clayton <a.clayton@nginx.com>
         Cc: Zhidao Hong <z.hong@f5.com>
         Cc: Andrei Zeliankou <zelenkov@nginx.com>
 8:  86f620d6 =  8:  cd95052a Static: Honor (MIME) "types" in "index"
 9:  a5a0beba =  9:  a9b1f639 Static: Unconditionally get the MIME type

alejandro-colomar commented 5 months ago

v6c changes:

Squash 2 and 4 (and rename) [@andrey-zelenkov ]

$ git range-diff master gh/mtypes mtypes 
 1:  6d2eb657 =  1:  6d2eb657 Static: Use shr instead of ctx->share
 2:  1a7048f8 !  2:  55665172 Static: Use a common variable in both index and non-index code
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    Static: Use a common variable in both index and non-index code
    +    Static: Store the full path name in a nxt_str_t

         Refactor.

    @@ src/nxt_http_static.c: nxt_http_static_send_ready(nxt_task_t *task, void *obj, v
              if (conf->types == NULL) {
                  nxt_str_null(&exten);

    +         } else {
    +-            nxt_http_static_extract_extension(shr, &exten);
    ++            nxt_http_static_extract_extension(path, &exten);
    +             mtype = nxt_http_static_mtype_get(&rtcf->mtypes_hash, &exten);
    + 
    +             ret = nxt_http_route_test_rule(r, conf->types, mtype->start,
     @@ src/nxt_http_static.c: nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
                      return;
                  }
    @@ src/nxt_http_static.c: nxt_http_static_send_ready(nxt_task_t *task, void *obj, v
          nxt_memzero(&file, sizeof(nxt_file_t));

          file.name = fname;
    +@@ src/nxt_http_static.c: nxt_http_static_send_ready(nxt_task_t *task, void *obj, void *data)
    +                               - p;
    + 
    +         if (exten.start == NULL) {
    +-            nxt_http_static_extract_extension(shr, &exten);
    ++            nxt_http_static_extract_extension(path, &exten);
    +         }
    + 
    +         if (mtype == NULL) {
 3:  3a4c8a91 =  3:  f18b7c54 Static: Split conditional
 4:  92d94903 <  -:  -------- Static: Use path instead of shr for extracting the extension
 5:  8fa7a02e =  4:  ebd48a1e Static: Don't include the terminating null byte in exten
 6:  2fd35728 =  5:  a59afb67 Static: Unconditionally extract the extension
 7:  c8c4c70a =  6:  5ea5501e Static: Terminate string with a null byte
 8:  cd95052a =  7:  a45ac0fe Static: Honor (MIME) "types" in "index"
 9:  a9b1f639 =  8:  be8df3c4 Static: Unconditionally get the MIME type

alejandro-colomar commented 5 months ago

v7 changes:

Don't invert conditinal logic. It was useful with the first versions, but after fixing the bugs regarding NUL, it results in a simpler diff without inverting it.

@andrey-zelenkov , I hope these changes reduce significantly your concerns regarding the state between commits.

The range-diff is horrible, but here's the interdiff:

$ git diff be8df3c4..4b436775
$

hongzhidao commented 5 months ago

Hi, Thanks for the patches, The commit https://github.com/nginx/unit/commit/c5220944d2acdb912c129fc82ac8a83d24e9845d was mentioned several times and was reported as a bug. We have unit tests on it and it passed. I wonder if you found it by testing it or by reading the source code.

alejandro-colomar commented 5 months ago

Hi, Thanks for the patches, The commit c522094 was mentioned several times and was reported as a bug. We have unit tests on it and it passed. I wonder if you found it by testing it or by reading the source code.

Hi @hongzhidao ,

I found it by reading the source code, and the diffs, as documented in the commit messages.

I must say I'm surprised that the tests pass, and that static sharing works. My guess is that some structure contains a 0 in the right place in memory, and so open(2) reads a seemingly-valid string.

hongzhidao commented 5 months ago

I'd suggest testing it before claiming it's a bug. Or don't call it a bug in a specific word before sure it's definetely a bug, it may misunderstand others.

alejandro-colomar commented 5 months ago

v7b changes:

Squash 1 and 2 [@ac000 ]

$ git range-diff master gh/mtypes mtypes 
1:  6d2eb657 < -:  -------- Static: Use shr instead of ctx->share
2:  55665172 ! 1:  ad561500 Static: Store the full path name in a nxt_str_t
    @@ Commit message

         Also, add myself to copyright.

    +    BTW, ctx->share is the same as shr.  It was probably a leftover of some
    +    old refactor.
    +
         Cc: Andrew Clayton <a.clayton@nginx.com>
         Cc: Andrei Zeliankou <zelenkov@nginx.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
    @@ src/nxt_http_static.c: nxt_http_static_send_ready(nxt_task_t *task, void *obj, v
                  }
              }
     -
    --        fname = shr->start;
    +-        fname = ctx->share.start;
          }

     +    fname = path->start;
3:  69e70ef8 = 2:  789fd42b Static: Split conditional
4:  fac74af5 = 3:  836dfbf9 Static: Don't include the terminating null byte in exten
5:  a441228c = 4:  a714064a Static: Unconditionally extract the extension
6:  a6f354ca = 5:  acda6edb Static: Terminate string with a null byte
7:  801ddfe9 = 6:  8a4b1c47 Static: Honor (MIME) "types" in "index"
8:  4b436775 = 7:  8c551396 Static: Unconditionally get the MIME type

alejandro-colomar commented 5 months ago

v8 changes:

Squash 2 and 4 (with some reordering, the squash doesn't look bad)

$ git range-diff master gh/mtypes mtypes 
1:  ad561500 = 1:  ad561500 Static: Store the full path name in a nxt_str_t
2:  789fd42b < -:  -------- Static: Split conditional
3:  836dfbf9 ! 2:  019b0814 Static: Don't include the terminating null byte in exten
    @@ src/nxt_http_static.c: nxt_http_static_send_ready(nxt_task_t *task, void *obj, v
     +
          } else {
              path = shr;
    -     }
    + 
     @@ src/nxt_http_static.c: nxt_http_static_extract_extension(nxt_str_t *path, nxt_str_t *exten)

      extension:
4:  a714064a ! 3:  43eda1b8 Static: Unconditionally extract the extension
    @@ src/nxt_http_static.c: nxt_http_static_send_ready(nxt_task_t *task, void *obj, v
     -
          } else {
              path = shr;
    -     }
    ++    }

    --    if (shr->start[shr->length - 1] != '/') {
     -        if (conf->types == NULL) {
     -            nxt_str_null(&exten);
     +    nxt_http_static_extract_extension(path, &exten);
5:  acda6edb = 4:  eb2ad841 Static: Terminate string with a null byte
6:  8a4b1c47 = 5:  66db7eb0 Static: Honor (MIME) "types" in "index"
7:  8c551396 = 6:  2ae41725 Static: Unconditionally get the MIME type

alejandro-colomar commented 5 months ago

After reasoning with @VBart , changing the behavior is probably worse than keeping current behavior. Let's drop this PR (but keep it in mind in case it becomes relevant again). I've open a new one with only the bug fixes.