nginx: fix failure to serve on aliases with regex captures #66532

yorickvP · 2019-08-12T14:57:18Z

Motivation for this change

It turns out the previous patch did not take in consideration that you could do alias a/$1/b. This means realpath would return NULL on this and nginx would return a 500 error instead.

Things done

This PR does the following:

add a test for this case
use the actual file path instead of the configured root dir, so symlinks to outside of the nix store work as expected
do not throw out the whole http request if realpath returns NULL

Notify maintainers

cc @edef1c @yegortimoshenko @aszlig @grahamc @domenkozar

mmahut · 2019-08-12T19:32:14Z

@GrahamcOfBorg test nginx

7c6f434c

But maybe use git mv on the patch — the patch gets complicated and it is hard to see the changes to the patch…

lukateras

Would be able to review ~~after git mv~~ when there is a line-by-line diff.

yorickvP · 2019-08-13T09:05:54Z

Git mv is a shorthand for git add && git rm. It won't help rename detection. I can split this patch into a change and a rename commit, though.

…

On Tue, Aug 13, 2019, 10:47 Yegor Timoshenko ***@***.***> wrote: ***@***.**** commented on this pull request. Would be able to review after git mv. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#66532?email_source=notifications&email_token=AAE57JG2N6UGVP3YMPRFQFTQEJYLDA5CNFSM4ILCCUGKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBLQXIY#pullrequestreview-274140067>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE57JBIDUHTJCUMLUJ2UALQEJYLDANCNFSM4ILCCUGA> .

lukateras · 2019-08-13T10:22:49Z

It seems that my choice of file name and format was unfortunate, maybe this could be better off as a diff (instead of a patch) and without Nginx version in the file name?

aszlig · 2019-08-13T11:17:49Z

pkgs/servers/http/nginx/nix-etag-1.16.0.patch

+ ngx_int_t ngx_http_set_content_type(ngx_http_request_t *r);
+ void ngx_http_set_exten(ngx_http_request_t *r);
+-ngx_int_t ngx_http_set_etag(ngx_http_request_t *r);
+ngx_int_t ngx_http_set_etag(ngx_http_request_t *r, ngx_str_t *path);


This is a bit unfortunate here, because this changes the API for third-party extension modules.

Same concern, I don't think we should ever change API of any programs that we patch (i.e. no headers changes).

aszlig · 2019-08-13T11:36:37Z

pkgs/servers/http/nginx/nix-etag-1.16.0.patch

+
+        if (etag->value.data == NULL) {
+            etag->hash = 0;
+            return NGX_ERROR;


Memleak: ngx_free(real);

Addendum: The reason why this could memleak is when real is not NULL but it's not pointing to the Nix store.

aszlig · 2019-08-13T11:41:44Z

pkgs/servers/http/nginx/nix-etag-1.16.0.patch

+                          - etag->value.data;
+    }
+
+     r->headers_out.etag = etag;


Same here: ngx_free(real);

aszlig · 2019-08-13T11:43:23Z

pkgs/servers/http/nginx/nix-etag-1.16.0.patch

+        }
+
+        ngx_memcpy(etag->value.data, ptr1, etag->value.len);
+        ngx_free(real);


This isn't needed if you just move it past the else branch below.

aszlig

As mentioned by @yegortimoshenko, it would probably make sense to just use a plain diff and either keep the versioned name (it still applies on version 1.15.4) or drop it entirely.

Apart from the mentioned memleaks, the API change is something I'd try to avoid as we don't know which external modules people might use.

yorickvP · 2019-08-14T11:05:16Z

updated: renamed the patch file in separate commits. Fixed API compatibility. Fixed memleaks.
@aszlig @yegortimoshenko
261f85d is the relevant diff.

7c6f434c · 2019-08-14T18:26:07Z

I wonder if it makes sense to allow NULL path and provide the old external API as a wrapper.

yorickvP · 2019-08-14T20:15:59Z

@7c6f434c that's what I'm doing now, at least

7c6f434c · 2019-08-15T10:36:12Z

pkgs/servers/http/nginx/nix-etag-1.x.patch

+
+        if (etag->value.data == NULL) {
+            etag->hash = 0;
+            if (real != NULL) ngx_free(real);


In other places you don't do a NULL check (and apparently at least glibc version of free is OK with free(NULL)) — why is this place different?

I do a NULL check everywhere it could be null, which isn't in the other branch of the if

ngx_free is just an alias for free of the corresponding C library, from http://www.open-std.org/JTC1/SC22/wg14/www/docs/n1124.pdf section 7.20.3.2:

If ptr is a null pointer, no action occurs.

So we don't need to do explicit checks like this.

7c6f434c · 2019-08-15T15:28:06Z

pkgs/servers/http/nginx/nix-etag-1.x.patch

+
+     etag = ngx_list_push(&r->headers_out.headers);
+     if (etag == NULL) {
+        ngx_free(real);


If you do NULL checks before free, this seems reachable without setting real when path is NULL

domenkozar · 2019-12-02T14:13:55Z

@yorickvP do you plan to address feedback?

cransom · 2019-12-04T16:12:07Z

I'd like to see this get more attention/merged. Without it, my nginx config was unable to serve user directories (eg example.com/~user/foo) which caused some good head scratching for a while until I found the PR.

yorickvP · 2019-12-04T16:39:23Z

Yes! I'll try to get to this this week. Op wo 4 dec. 2019 om 23:12 schreef Casey Ransom <notifications@github.com>:

…

I'd like to see this get more attention/merged. Without it, my nginx config was unable to serve user directories (eg example.com/~user/foo) which caused some good head scratching for a while until I found the PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#66532?email_source=notifications&email_token=AAE57JFND2L6QNZRSJ7LVQTQW7JFRA5CNFSM4ILCCUGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF5RPMA#issuecomment-561715120>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE57JFQ6S4WBCILUEWZYQTQW7JFRANCNFSM4ILCCUGA> .

jglukasik · 2020-02-10T00:09:34Z

+1 on this getting more attention, this breaks my dynamic subdomain setup. I've been able to workaround this by overriding the patches attribute in the nginx package used by the service. Sharing my relevant configuration.nix lines below in case anyone else is having similar troubles:

services = {
  nginx = {
    enable = true;

    # don't apply patches, they break my subdomain setup
    # https://github.com/NixOS/nixpkgs/pull/66532
    package = (pkgs.nginx.overrideAttrs (oldAttrs: {
      name = "nginx-temp-no-patches";
      patches = [];
    }));

    virtualHosts = {
      "subs.ex.example.com" = {
        serverName = "~^(?<sub>.+)\\.ex\\.example\\.com$";
        root = "/var/www/ex.example.com/$sub";
      };
    };
  };
};

aszlig · 2020-02-21T20:57:02Z

@jglukasik: See also #80671.

@devhell

While our ETag patch works pretty fine if it comes to serving data off store paths, it unfortunately broke something that might be a bit more common, namely when using regexes to extract path components of location directives for example. Recently, @devhell has reported a bug with a nginx location directive like this: location ~^/\~([a-z0-9_]+)(/.*)?$" { alias /home/$1/public_html$2; } While this might look harmless at first glance, it does however cause issues with our ETag patch. The alias directive gets broken up by nginx like this: *2 http script copy: "/home/" *2 http script capture: "foo" *2 http script copy: "/public_html/" *2 http script capture: "bar.txt" In our patch however, we use realpath(3) to get the canonicalised path from ngx_http_core_loc_conf_s.root, which returns the *configured* value from the root or alias directive. So in the example above, realpath(3) boils down to the following syscalls: lstat("/home", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 lstat("/home/$1", 0x7ffd08da6f60) = -1 ENOENT (No such file or directory) During my review[1] of the initial patch, I didn't actually notice that what we're doing here is returning NGX_ERROR if the realpath(3) call fails, which in turn causes an HTTP 500 error. Since our patch actually made the canonicalisation (and thus additional syscalls) necessary, we really shouldn't introduce an additional error so let's - at least for now - silently skip return value if realpath(3) has failed. However since we're using the unaltered root from the config we have another issue, consider this root: /nix/store/...-abcde/$1 Calling realpath(3) on this path will fail (except if there's a file called "$1" of course), so even this fix is not enough because it results in the ETag not being set to the store path hash. While this is very ugly and we should fix this very soon, it's not as serious as getting HTTP 500 errors for serving static files. I added a small NixOS VM test, which uses the example above as a regression test. It seems that my memory is failing these days, since apparently I *knew* about this issue since digging for existing issues in nixpkgs, I found this similar pull request which I even reviewed: NixOS#66532 However, since the comments weren't addressed and the author hasn't responded to the pull request, I decided to keep this very commit and do a follow-up pull request. [1]: NixOS#48337 Signed-off-by: aszlig <aszlig@nix.build> Reported-by: @devhell (cherry picked from commit f6c7635)

@devhell

While our ETag patch works pretty fine if it comes to serving data off store paths, it unfortunately broke something that might be a bit more common, namely when using regexes to extract path components of location directives for example. Recently, @devhell has reported a bug with a nginx location directive like this: location ~^/\~([a-z0-9_]+)(/.*)?$" { alias /home/$1/public_html$2; } While this might look harmless at first glance, it does however cause issues with our ETag patch. The alias directive gets broken up by nginx like this: *2 http script copy: "/home/" *2 http script capture: "foo" *2 http script copy: "/public_html/" *2 http script capture: "bar.txt" In our patch however, we use realpath(3) to get the canonicalised path from ngx_http_core_loc_conf_s.root, which returns the *configured* value from the root or alias directive. So in the example above, realpath(3) boils down to the following syscalls: lstat("/home", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 lstat("/home/$1", 0x7ffd08da6f60) = -1 ENOENT (No such file or directory) During my review[1] of the initial patch, I didn't actually notice that what we're doing here is returning NGX_ERROR if the realpath(3) call fails, which in turn causes an HTTP 500 error. Since our patch actually made the canonicalisation (and thus additional syscalls) necessary, we really shouldn't introduce an additional error so let's - at least for now - silently skip return value if realpath(3) has failed. However since we're using the unaltered root from the config we have another issue, consider this root: /nix/store/...-abcde/$1 Calling realpath(3) on this path will fail (except if there's a file called "$1" of course), so even this fix is not enough because it results in the ETag not being set to the store path hash. While this is very ugly and we should fix this very soon, it's not as serious as getting HTTP 500 errors for serving static files. I added a small NixOS VM test, which uses the example above as a regression test. It seems that my memory is failing these days, since apparently I *knew* about this issue since digging for existing issues in nixpkgs, I found this similar pull request which I even reviewed: #66532 However, since the comments weren't addressed and the author hasn't responded to the pull request, I decided to keep this very commit and do a follow-up pull request. [1]: #48337 Signed-off-by: aszlig <aszlig@nix.build> Reported-by: @devhell Acked-by: @7c6f434c Acked-by: @yorickvP Merges: #80671 Fixes: #66532 (cherry picked from commit e1d63ad)

@devhell

While our ETag patch works pretty fine if it comes to serving data off store paths, it unfortunately broke something that might be a bit more common, namely when using regexes to extract path components of location directives for example. Recently, @devhell has reported a bug with a nginx location directive like this: location ~^/\~([a-z0-9_]+)(/.*)?$" { alias /home/$1/public_html$2; } While this might look harmless at first glance, it does however cause issues with our ETag patch. The alias directive gets broken up by nginx like this: *2 http script copy: "/home/" *2 http script capture: "foo" *2 http script copy: "/public_html/" *2 http script capture: "bar.txt" In our patch however, we use realpath(3) to get the canonicalised path from ngx_http_core_loc_conf_s.root, which returns the *configured* value from the root or alias directive. So in the example above, realpath(3) boils down to the following syscalls: lstat("/home", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 lstat("/home/$1", 0x7ffd08da6f60) = -1 ENOENT (No such file or directory) During my review[1] of the initial patch, I didn't actually notice that what we're doing here is returning NGX_ERROR if the realpath(3) call fails, which in turn causes an HTTP 500 error. Since our patch actually made the canonicalisation (and thus additional syscalls) necessary, we really shouldn't introduce an additional error so let's - at least for now - silently skip return value if realpath(3) has failed. However since we're using the unaltered root from the config we have another issue, consider this root: /nix/store/...-abcde/$1 Calling realpath(3) on this path will fail (except if there's a file called "$1" of course), so even this fix is not enough because it results in the ETag not being set to the store path hash. While this is very ugly and we should fix this very soon, it's not as serious as getting HTTP 500 errors for serving static files. I added a small NixOS VM test, which uses the example above as a regression test. It seems that my memory is failing these days, since apparently I *knew* about this issue since digging for existing issues in nixpkgs, I found this similar pull request which I even reviewed: #66532 However, since the comments weren't addressed and the author hasn't responded to the pull request, I decided to keep this very commit and do a follow-up pull request. [1]: #48337 Signed-off-by: aszlig <aszlig@nix.build> Reported-by: @devhell Acked-by: @7c6f434c Acked-by: @yorickvP Merges: #80671 Fixes: #66532 (cherry picked from commit e1d63ad)

@devhell

While our ETag patch works pretty fine if it comes to serving data off store paths, it unfortunately broke something that might be a bit more common, namely when using regexes to extract path components of location directives for example. Recently, @devhell has reported a bug with a nginx location directive like this: location ~^/\~([a-z0-9_]+)(/.*)?$" { alias /home/$1/public_html$2; } While this might look harmless at first glance, it does however cause issues with our ETag patch. The alias directive gets broken up by nginx like this: *2 http script copy: "/home/" *2 http script capture: "foo" *2 http script copy: "/public_html/" *2 http script capture: "bar.txt" In our patch however, we use realpath(3) to get the canonicalised path from ngx_http_core_loc_conf_s.root, which returns the *configured* value from the root or alias directive. So in the example above, realpath(3) boils down to the following syscalls: lstat("/home", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0 lstat("/home/$1", 0x7ffd08da6f60) = -1 ENOENT (No such file or directory) During my review[1] of the initial patch, I didn't actually notice that what we're doing here is returning NGX_ERROR if the realpath(3) call fails, which in turn causes an HTTP 500 error. Since our patch actually made the canonicalisation (and thus additional syscalls) necessary, we really shouldn't introduce an additional error so let's - at least for now - silently skip return value if realpath(3) has failed. However since we're using the unaltered root from the config we have another issue, consider this root: /nix/store/...-abcde/$1 Calling realpath(3) on this path will fail (except if there's a file called "$1" of course), so even this fix is not enough because it results in the ETag not being set to the store path hash. While this is very ugly and we should fix this very soon, it's not as serious as getting HTTP 500 errors for serving static files. I added a small NixOS VM test, which uses the example above as a regression test. It seems that my memory is failing these days, since apparently I *knew* about this issue since digging for existing issues in nixpkgs, I found this similar pull request which I even reviewed: NixOS#66532 However, since the comments weren't addressed and the author hasn't responded to the pull request, I decided to keep this very commit and do a follow-up pull request. [1]: NixOS#48337 Signed-off-by: aszlig <aszlig@nix.build> Reported-by: @devhell Acked-by: @7c6f434c Acked-by: @yorickvP Merges: NixOS#80671 Fixes: NixOS#66532 (cherry picked from commit e1d63ad)

ofborg bot added the 6.topic: nixos label Aug 12, 2019

yorickvP force-pushed the nginx-etag-fix branch from e0ea75c to 5702f25 Compare August 12, 2019 15:19

ofborg bot requested review from 7c6f434c, fpletz and thoughtpolice August 12, 2019 15:34

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 12, 2019

7c6f434c approved these changes Aug 13, 2019

View reviewed changes

lukateras reviewed Aug 13, 2019

View reviewed changes

aszlig reviewed Aug 13, 2019

View reviewed changes

aszlig requested changes Aug 13, 2019

View reviewed changes

yorickvP added 3 commits August 14, 2019 12:51

nixos/tests/nginx: add (failing) test for regex captured aliases

803cc28

nginx: rename nix-etag-1.15.4.patch -> nix-etag.1.1x.patch

c988c91

nginx: use actual file path to calculate etag by nix hash

261f85d

yorickvP force-pushed the nginx-etag-fix branch from 5702f25 to 261f85d Compare August 14, 2019 11:04

ofborg bot requested a review from 7c6f434c August 14, 2019 11:20

7c6f434c reviewed Aug 15, 2019

View reviewed changes

aszlig mentioned this pull request Feb 20, 2020

nginx: Fix ETag patch to ignore realpath(3) error #80671

Closed

aszlig closed this in e1d63ad Mar 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nginx: fix failure to serve on aliases with regex captures #66532

nginx: fix failure to serve on aliases with regex captures #66532

yorickvP commented Aug 12, 2019

mmahut commented Aug 12, 2019

7c6f434c left a comment

lukateras left a comment •

edited

Loading

yorickvP commented Aug 13, 2019 via email

lukateras commented Aug 13, 2019

aszlig Aug 13, 2019

lukateras Aug 13, 2019

aszlig Aug 13, 2019 •

edited

Loading

aszlig Aug 13, 2019

aszlig Aug 13, 2019

aszlig left a comment

yorickvP commented Aug 14, 2019

7c6f434c commented Aug 14, 2019

yorickvP commented Aug 14, 2019

7c6f434c Aug 15, 2019

yorickvP Aug 15, 2019

aszlig Aug 15, 2019

7c6f434c Aug 15, 2019

domenkozar commented Dec 2, 2019

cransom commented Dec 4, 2019

yorickvP commented Dec 4, 2019 via email

jglukasik commented Feb 10, 2020

aszlig commented Feb 21, 2020

nginx: fix failure to serve on aliases with regex captures #66532

nginx: fix failure to serve on aliases with regex captures #66532

Conversation

yorickvP commented Aug 12, 2019

Motivation for this change

Things done

Notify maintainers

mmahut commented Aug 12, 2019

7c6f434c left a comment

Choose a reason for hiding this comment

lukateras left a comment • edited Loading

Choose a reason for hiding this comment

yorickvP commented Aug 13, 2019 via email

lukateras commented Aug 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aszlig Aug 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aszlig left a comment

Choose a reason for hiding this comment

yorickvP commented Aug 14, 2019

7c6f434c commented Aug 14, 2019

yorickvP commented Aug 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenkozar commented Dec 2, 2019

cransom commented Dec 4, 2019

yorickvP commented Dec 4, 2019 via email

jglukasik commented Feb 10, 2020

aszlig commented Feb 21, 2020

lukateras left a comment •

edited

Loading

aszlig Aug 13, 2019 •

edited

Loading