------------------------------------------------------------ revno: 13844 revision-id: squid3@treenet.co.nz-20150605233834-v3wmgui9ue5kubzg parent: squid3@treenet.co.nz-20150605233010-191la639zs3rurx9 fixes bug: http://bugs.squid-cache.org/show_bug.cgi?id=3875 committer: Amos Jeffries branch nick: 3.5 timestamp: Fri 2015-06-05 16:38:34 -0700 message: Bug 3875: bad mimeLoadIconFile error handling Improve the MimeIcon reliability when filesystem I/O errors or others cause the icon data to not be loadable. The loading process is re-worked to guarantee that once the MimeIon::created callback occurs it will result in a valid StoreEntry in the cache representing the wanted icon. * If the image can be loaded without any issues it will be placed in the cache as a 200 response. * If errors prevent the image being loaded or necessary parameters (size and mtime) being known a 204 object will be placed into the cache. NP: There is no clear agreement on 204 being 'the best' status for this case. 500 Internal Error is also appropriate. I have use 204 since: * the bug is not in the clients request (eliminating 400, 404, etc), * a 500 would be revealing details about server internals unnecessarily often and incur extra complexity creating the error page. * 204 also avoids needing to send Content-Length, Cache-Control header and body object (bandwidth saving over 500 status). NP: This started with just correcting the errno usage, but other bugs promptly started appearing once I got to seriously testing this load process. So far it fixes: * several assertions resulting from StoreEntry being left invalid in cache limbo beween created hash entries and valid mem_obj data. * repeated attempts on startup to load absent icons files which dont exist in the filesystem. * buffer overfow on misconfigured or corrupt mime.conf file entries * incorrect debugs messages about file I/O errors * large error pages delivered when icons not installed (when it does not assert from the StoreEntry) ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20150605233834-v3wmgui9ue5kubzg # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: 1b1f371bf6de57dd028f1d3669fb05549e46a5bb # timestamp: 2015-06-05 23:51:00 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: squid3@treenet.co.nz-20150605233010-\ # 191la639zs3rurx9 # # Begin patch === modified file 'src/mime.cc' --- src/mime.cc 2015-01-13 09:13:49 +0000 +++ src/mime.cc 2015-06-05 23:38:34 +0000 @@ -40,11 +40,14 @@ public: explicit MimeIcon(const char *aName); ~MimeIcon(); + MEMPROXY_CLASS(MimeIcon); + void setName(char const *); char const * getName() const; void load(); - void created(StoreEntry *newEntry); - MEMPROXY_CLASS(MimeIcon); + + /* StoreClient API */ + virtual void created(StoreEntry *); private: const char *icon_; @@ -361,32 +364,43 @@ } void -MimeIcon::created (StoreEntry *newEntry) +MimeIcon::created(StoreEntry *newEntry) { /* if the icon is already in the store, do nothing */ if (!newEntry->isNull()) return; - - int fd; - int n; + // XXX: if a 204 is cached due to earlier load 'failure' we should try to reload. + + // default is a 200 object with image data. + // set to the backup value of 204 on image loading errors + Http::StatusCode status = Http::scOkay; + + static char path[MAXPATHLEN]; + *path = 0; + if (snprintf(path, sizeof(path)-1, "%s/%s", Config.icons.directory, icon_) < 0) { + debugs(25, DBG_CRITICAL, "ERROR: icon file '" << Config.icons.directory << "/" << icon_ << "' path is longer than " << MAXPATHLEN << " bytes"); + status = Http::scNoContent; + } + + int fd = -1; + errno = 0; + if (status == Http::scOkay && (fd = file_open(path, O_RDONLY | O_BINARY)) < 0) { + int xerrno = errno; + debugs(25, DBG_CRITICAL, "ERROR: opening icon file " << path << ": " << xstrerr(xerrno)); + status = Http::scNoContent; + } + + struct stat sb; + errno = 0; + if (status == Http::scOkay && fstat(fd, &sb) < 0) { + int xerrno = errno; + debugs(25, DBG_CRITICAL, "ERROR: opening icon file " << path << " FD " << fd << ", fstat error " << xstrerr(xerrno)); + file_close(fd); + status = Http::scNoContent; + } + + // fill newEntry with a canned 2xx response object RequestFlags flags; - struct stat sb; - LOCAL_ARRAY(char, path, MAXPATHLEN); - char *buf; - - snprintf(path, MAXPATHLEN, "%s/%s", Config.icons.directory, icon_); - - fd = file_open(path, O_RDONLY | O_BINARY); - if (fd < 0) { - debugs(25, DBG_CRITICAL, "Problem opening icon file " << path << ": " << xstrerror()); - return; - } - if (fstat(fd, &sb) < 0) { - debugs(25, DBG_CRITICAL, "Problem opening icon file. Fd: " << fd << ", fstat error " << xstrerror()); - file_close(fd); - return; - } - flags.cachable = true; StoreEntry *e = storeCreateEntry(url_,url_,flags,Http::METHOD_GET); assert(e != NULL); @@ -396,30 +410,37 @@ HttpRequest *r = HttpRequest::CreateFromUrl(url_); if (NULL == r) - fatal("mimeLoadIcon: cannot parse internal URL"); + fatalf("mimeLoadIcon: cannot parse internal URL: %s", url_); e->mem_obj->request = r; HTTPMSGLOCK(e->mem_obj->request); HttpReply *reply = new HttpReply; - reply->setHeaders(Http::scOkay, NULL, mimeGetContentType(icon_), sb.st_size, sb.st_mtime, -1); + if (status == Http::scNoContent) + reply->setHeaders(status, NULL, NULL, 0, -1, -1); + else + reply->setHeaders(status, NULL, mimeGetContentType(icon_), sb.st_size, sb.st_mtime, -1); reply->cache_control = new HttpHdrCc(); reply->cache_control->maxAge(86400); reply->header.putCc(reply->cache_control); e->replaceHttpReply(reply); - /* read the file into the buffer and append it to store */ - buf = (char *)memAllocate(MEM_4K_BUF); - while ((n = FD_READ_METHOD(fd, buf, 4096)) > 0) - e->append(buf, n); - - file_close(fd); + if (status == Http::scOkay) { + /* read the file into the buffer and append it to store */ + int n; + char *buf = (char *)memAllocate(MEM_4K_BUF); + while ((n = FD_READ_METHOD(fd, buf, sizeof(*buf))) > 0) + e->append(buf, n); + + file_close(fd); + memFree(buf, MEM_4K_BUF); + } + e->flush(); e->complete(); e->timestampsSet(); e->unlock("MimeIcon::created"); - memFree(buf, MEM_4K_BUF); debugs(25, 3, "Loaded icon " << url_); }