[squid-dev] Do not load icons one character at a time

[Alex Rousskov]

On 05/21/2016 03:09 AM, Amos Jeffries wrote:
> On 21/05/2016 6:25 a.m., Alex Rousskov wrote:
>>>   Since trunk r14100 (Bug 3875: bad mimeLoadIconFile error handling), each
>>>   icon was read from disk and written to Store one character at a time.


> -        char *buf = (char *)memAllocate(MEM_4K_BUF);
> -        while ((n = FD_READ_METHOD(fd, buf, sizeof(*buf))) > 0)


> Do you know where it is coming from exactly?
>  sizeof(*buf) on dynamic arrays should be the array length. Not the char
> size.

... because the compiler adds secret code that stores sizes of all
dynamically allocated buffers in a secret hash and then looks that hash
up whenever somebody calls sizeof() for any pointer, including sizeof()
calls in another library compiled by another compiler and running in
another thread? Think about it.

sizeof(expression) always returns the size of expression's type. The
only caveat to keep in mind when dealing with sizeof is that arrays do
not decay to pointers when passed to sizeof.

sizeof("foo") returning 4 confuses developers, but it is not magical if
you know the type of literal strings like "foo" (it is an array of
chars, and not a char pointer).

So, in trunk r14100 case, sizeof(*buf) means sizeof(char) which is 1.

References:
  http://stackoverflow.com/questions/1392200/sizeof-string-literal
  http://en.cppreference.com/w/cpp/language/sizeof



If you recall any other place where you used an expression like
sizeof(*buf) to mean "the size of the dynamically allocated buffer that
buf points to", please fix those as well.



>  sizeof(buf) is the 32-bit/64-bit size of pointers in the build. So we
> can't use that.

Correct.


> We could hard-code the 4KB size here for 3.5. But I'd rather learn what
> was wrong.

I recommend using an array for v3.5:

    char buf[4096];

A fixed-size array is simple, fast, exception-safe, does not need
dynamic allocation/cleanup, and allows you to use sizeof(buf) instead of
repeating 4096 or naming that constant. 4KB stack objects are fine in
non-recursive, non-kernel contexts. A C array is less safe than
std::array, but we can leave with that risk in this context.

I do not know why you have chosen the complex route of dynamically
allocating a 4KiB buffer. Perhaps you were trying to avoid writing
"4096"? There is no shame in writing that, as long as you write it only
once for each distinct purpose.


HTH,

Alex.