[squid-dev] [PATCH] Bug 3875 MimeIcon error handling
Amos Jeffries
squid3 at treenet.co.nz
Wed May 20 15:22:45 UTC 2015
This is an attempt to 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.
I have selected 204 as the representation of errors 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 set
and emit Content-Length header, and just enough different from 200 to
use as a special-case test.
NP: It 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.
* on startup 6+ repeated attempts 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 doesnt
assert from the StoreEntry)
Amos
-------------- next part --------------
=== modified file 'src/mime.cc'
--- src/mime.cc 2015-01-13 07:25:36 +0000
+++ src/mime.cc 2015-05-20 14:35:05 +0000
@@ -25,41 +25,43 @@
#include "StoreClient.h"
#if HAVE_SYS_STAT_H
#include <sys/stat.h>
#endif
/* forward declarations */
static void mimeFreeMemory(void);
static char const *mimeGetIcon(const char *fn);
class MimeIcon : public StoreClient
{
MEMPROXY_CLASS(MimeIcon);
public:
explicit MimeIcon(const char *aName);
~MimeIcon();
void setName(char const *);
char const * getName() const;
void load();
- void created(StoreEntry *newEntry);
+
+ /* StoreClient API */
+ virtual void created(StoreEntry *);
private:
const char *icon_;
char *url_;
};
class MimeEntry
{
MEMPROXY_CLASS(MimeEntry);
public:
explicit MimeEntry(const char *aPattern, const regex_t &compiledPattern,
const char *aContentType,
const char *aContentEncoding, const char *aTransferMode,
bool optionViewEnable, bool optionDownloadEnable,
const char *anIconName);
~MimeEntry();
const char *pattern;
regex_t compiled_pattern;
@@ -341,99 +343,117 @@
while ((m = MimeTable)) {
MimeTable = m->next;
delete m;
}
MimeTableTail = &MimeTable;
}
void
MimeIcon::load()
{
const char *type = mimeGetContentType(icon_);
if (type == NULL)
fatal("Unknown icon format while reading mime.conf\n");
StoreEntry::getPublic(this, url_, Http::METHOD_GET);
}
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;
-
- snprintf(path, MAXPATHLEN, "%s/%s", Config.icons.directory, icon_);
+ // 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;
+ }
- 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);
EBIT_SET(e->flags, ENTRY_SPECIAL);
e->setPublicKey();
e->buffer();
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_);
}
MimeEntry::~MimeEntry()
{
xfree(pattern);
xfree(content_type);
xfree(content_encoding);
regfree(&compiled_pattern);
}
MimeEntry::MimeEntry(const char *aPattern, const regex_t &compiledPattern,
const char *aContentType, const char *aContentEncoding,
const char *aTransferMode, bool optionViewEnable,
bool optionDownloadEnable, const char *anIconName) :
pattern(xstrdup(aPattern)),
compiled_pattern(compiledPattern),
content_type(xstrdup(aContentType)),
content_encoding(xstrdup(aContentEncoding)),
view_option(optionViewEnable),
More information about the squid-dev
mailing list