Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#1630 closed defect (invalid)

open_file_cache loses trailing newline on changed files

Reported by: Sam Bull Owned by:
Priority: major Milestone:
Component: nginx-core Version: 1.10.x
Keywords: Cc:
uname -a: Linux server 4.9.0-8-amd64 #1 SMP Debian 4.9.110-3+deb9u4 (2018-08-21) x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.10.3
built with OpenSSL 1.1.0f 25 May 2017
TLS SNI support enabled
configure arguments: --with-cc-opt='-g -O2 -fdebug-prefix-map=/build/nginx-2tpxfc/nginx-1.10.3=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2' --with-ld-opt='-Wl,-z,relro -Wl,-z,now' --prefix=/usr/share/nginx --conf-path=/etc/nginx/nginx.conf --http-log-path=/var/log/nginx/access.log --error-log-path=/var/log/nginx/error.log --lock-path=/var/lock/nginx.lock --pid-path=/run/nginx.pid --modules-path=/usr/lib/nginx/modules --http-client-body-temp-path=/var/lib/nginx/body --http-fastcgi-temp-path=/var/lib/nginx/fastcgi --http-proxy-temp-path=/var/lib/nginx/proxy --http-scgi-temp-path=/var/lib/nginx/scgi --http-uwsgi-temp-path=/var/lib/nginx/uwsgi --with-debug --with-pcre-jit --with-ipv6 --with-http_ssl_module --with-http_stub_status_module --with-http_realip_module --with-http_auth_request_module --with-http_v2_module --with-http_dav_module --with-http_slice_module --with-threads --with-http_gzip_static_module --without-http_browser_module --without-http_geo_module --without-http_limit_req_module --without-http_limit_conn_module --without-http_memcached_module --without-http_referer_module --without-http_split_clients_module --without-http_userid_module --add-dynamic-module=/build/nginx-2tpxfc/nginx-1.10.3/debian/modules/nginx-echo

Description

When using open_file_cache, if I modify a file, the server starts serving the new file without a trailing newline until the cache expires.

This causes issues with SRI, where the hash of the file (which includes a trailing newline) is created by the application and included in the integrity attribute. Because Nginx is serving a version of the file to the browser without the newline, this causes the SRI check to fail and the browser refuses to run the resource.

Change History (3)

comment:1 by Maxim Dounin, 6 years ago

Resolution: invalid
Status: newclosed

If you non-atomically modify a file being served by a web server, you create a race condition: if nginx opens a file and starts sending it before the modification, and continues sending after the modification, the client will receive a mix of the old and modified files. Using open_files_cache ensures that this race condition will happen in practice even under low load, as files are opened and stat()'ed long before they are actually sent to the client.

When the file is make smaller during a in-place modification, nginx is likely to notice this, and will log an pread() read only ... of ... alert as in #139, or sendfile() reported that ... was truncated at ... alert when using sendfile(). However, if the file is made bigger or the size is preserved, it is not possible to automatically detect this without additional checks (or at all), so you are likely to only observe the corruption when looking into the response contents.

The only proper solution is to avoid non-atomic file modifications. That is, you have to update files atomically instead: write a new file with a temporary name, and then use mv to atomically rename it to the correct name. With this approach, nginx will be able to continue serving the old file till it is no longer needed, eliminating the race condition.

If you are not able to use atomic file updates for some reason, consider switching open_file_cache off - this will make the race condition created by the non-atomic modifications less likely to manifest itself in practice. Note though that this won't eliminate the race condition, and it is still possible that a corrupted response will be returned if a modification happens in the wrong time. As outlined above, the only proper solution is to use atomic file updates instead.

comment:2 by Sam Bull, 6 years ago

OK, but would it also be possible to use something like inotify to get alerted when a file has changed and clear the modified file from the cache?

Expecting users over SFTP to create temporary files and then rename them seems a little too much, plus getting applications (like a CMS) to do the same when updating files seems extremely difficult.

I understand there is still a chance of the race condition occurring, but atleast with the inotify solution it would work as expected the vast majority of the time, rather than often having unexpected results as the current solution provides.

comment:3 by Maxim Dounin, 6 years ago

OK, but would it also be possible to use something like inotify to get alerted when a file has changed and clear the modified file from the cache?

There were some attempts in the past to introduce event-based invalidation in open_file_cache (activated via the open_file_cache_events directive, uses kqueue's EVFILT_VNODE). This work was never complete though, mostly because the open_file_cache itself provides little-to-no benefits except may be in some very specific workloads.

If you cannot enforce proper atomic file updates in your environment, just switch open_file_cache off, this will minimize possible damage from non-atomic file modifications.

Note: See TracTickets for help on using tickets.