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

[Amos Jeffries]

On 22/05/2016 5:03 a.m., Alex Rousskov wrote:
> 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.

IIRC you would be referring to the malloc block header. Which our pools
allocator does not have. Figures.

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

I dont recall unfortunately. Not many, if any.

Right now grep shows uses of "sizeof(*" in total for 3.5 at 149, and
trunk at 148. I'll start reviewing them all shortly.

> 
>>  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 already reverted it to the original "4096" constant that was there
previously before receiving this.

Not sure this is important enough to warrant a second change just for
the remainder of 3.5 lifetime. If you disagree feel free to apply a
commit to 3.5 branch for that.

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

Not sure why you are using the word "you" there. I'm only responsible
for the sizeof().

Its current form as a local scope buffer with dynamic allocation was
begun in rev.2394 by yourself with its data being copied/appended into
StoreEntry. It appears that you did so as a way to keep things
consistent around the experiments #if construct, but later removal of
that "#if 0" dead code did not touch the code left behind.

AFAIK, Duane's rev.1575 had used a dynamic allocation earlier because
Squid was generating the entire response message, headers and body alike
in a "buf = get_free_4k_page();" allocation and passing it wholesale to
the comm_write equivalent of the day.

Amos