From: Amos Jeffries <squid3@treenet.co.nz>
Date: Tue, 2 Jun 2015 23:27:26 +0000 (-0700)
Subject: Bug 3875: bad mimeLoadIconFile error handling
X-Git-Tag: merge-candidate-3-v1~94
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9671c6b15fa50b5acb24c63c80c29eac88b347bd;p=thirdparty%2Fsquid.git

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)
---

diff --git a/src/mime.cc b/src/mime.cc
index a39f9e240b..fd6f6b7e98 100644
--- a/src/mime.cc
+++ b/src/mime.cc
@@ -42,7 +42,9 @@ public:
     void setName(char const *);
     char const * getName() const;
     void load();
-    void created(StoreEntry *newEntry);
+
+    /* StoreClient API */
+    virtual void created(StoreEntry *);
 
 private:
     const char *icon_;
@@ -358,32 +360,43 @@ MimeIcon::load()
 }
 
 void
-MimeIcon::created (StoreEntry *newEntry)
+MimeIcon::created(StoreEntry *newEntry)
 {
     /* if the icon is already in the store, do nothing */
     if (!newEntry->isNull())
         return;
+    // XXX: if a 204 is cached due to earlier load 'failure' we should try to reload.
 
-    int fd;
-    int n;
-    RequestFlags flags;
-    struct stat sb;
-    LOCAL_ARRAY(char, path, MAXPATHLEN);
-    char *buf;
+    // default is a 200 object with image data.
+    // set to the backup value of 204 on image loading errors
+    Http::StatusCode status = Http::scOkay;
 
-    snprintf(path, MAXPATHLEN, "%s/%s", Config.icons.directory, icon_);
+    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;
+    }
 
-    fd = file_open(path, O_RDONLY | O_BINARY);
-    if (fd < 0) {
-        debugs(25, DBG_CRITICAL, "Problem opening icon file " << path << ": " << xstrerror());
-        return;
+    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;
     }
-    if (fstat(fd, &sb) < 0) {
-        debugs(25, DBG_CRITICAL, "Problem opening icon file. Fd: " << fd << ", fstat error " << xstrerror());
+
+    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);
-        return;
+        status = Http::scNoContent;
     }
 
+    // fill newEntry with a canned 2xx response object
+    RequestFlags flags;
     flags.cachable = true;
     StoreEntry *e = storeCreateEntry(url_,url_,flags,Http::METHOD_GET);
     assert(e != NULL);
@@ -393,30 +406,37 @@ MimeIcon::created (StoreEntry *newEntry)
     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);
+    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);
+    }
 
-    file_close(fd);
     e->flush();
     e->complete();
     e->timestampsSet();
     e->unlock("MimeIcon::created");
-    memFree(buf, MEM_4K_BUF);
     debugs(25, 3, "Loaded icon " << url_);
 }