Use va_copy() on all platforms; fixed a dangerous low-level bug

[yadij]

To improve cross-compilation support and to simplify code, rely on C++11
cstdarg header instead of ./configure-time va_copy() detection.

Using ./configure-time detection for va_copy() is dangerous because when
it does not work (e.g., during a poorly configured cross-compilation
attempt), Squid may crash if va_copy() was needed but was not detected.

See also: Bug 4821 and bug 753.

Also found and fixed a low-level bug: StoreEntry::vappendf() was not
using va_copy() because store.cc lacked VA_COPY #defines. The affected
code (900+ callers!) is used for cache manager responses and Gopher
gateway response compilation. If any of those calls required a buffer
larger than 4KB, the lack of those va_copy() calls could lead to crashes
and/or data corruption issues on platforms where va_copy() is required.

[rousskov]

Thank you for cleaning this up. Let's hope all environments we care about comply with the <cstdarg> part of the C++11 standard.

Please also remove unused VA_* macros from compat/stdvarargs.h:

git grep VA_

[rousskov]

If possible please move this declaration to where it is actually needed/used.

[rousskov]

one of which turned out to be badly broken already

If you can detail that a little in the PR description (i.e., the future commit message), please do so. It might be useful for future triage or, in the worst case, undoing of this change.

BTW, I polished PR description to explain why we are changing this, to document the hidden dangers of the old approach, and to adhere to formatting requirements for commit messages. Please adjust further as needed, of course.

[yadij]

@rousskov, Why did you re-write the description and title to describe things which this PR does not do?

[rousskov]

Why did you re-write the description and title to describe things which this PR does not do?

It is difficult to answer loaded questions without misleading the reader. I believe the current title and description match the PR, but please feel free to fix/adjust to your liking, of course. I should be able to restore the original values if you do not have them but want (to start from) them. Just let me know!

I am going to remove my approval until the future commit message is settled.

[rousskov]

Replacing approval with a comment until the content of the future commit message is settled.

[rousskov]

Replacing approval with a fake rejection until the content of the future commit message is settled. (Replacing with a comment has no effect, but I will try to remove the rejection afterwards).

[rousskov]

Replacing fake rejection with a comment until the content of the future commit message is settled.

Dismissing fake rejection using the nuclear option.

[yadij]

Regarding the description:

My initial text detailed the two major changes this PR bundles together (albeit sparse text). The focus and core of this PR is the removal of the VA_COPY macro from src/*.cc files. The removal of configure.ac checks was a nice side-effect only done as cleanup maintenance after the primary change(s). The code will be just as fixed in terms of bugs if that configure.ac is left untouched.

Your updated text appears to moved focus from the VA_COPY hacks (d0d42f0) and bug in store.cc (2ce8ee2), onto the configure.ac change which is the least significant part.

I simply seek to know why such a significant re-write was needed?

Regarding the remainder VA_ macros from compat/stdvarargs.h:
(sorry, github wont let me reply to that request directly)

I have looked into them and those macro definitions are not exactly unused. They do appear to be unused in Squid , however what they actually do is modify things inside the system headers on some older OS so that the code Squid does contain actually works. Those modified things are still relevant to at least the snmplib/ code using stdargs.h include and maybe other old C includes used elsewhere - its all hidden behind the fact that these are in a global compat/* header.

So, if you wish I can remove them in a followup PR as an attempt to a) reduce the code for outdated OS , and b) trigger bug reports from any OS which is still affected by that kind of bug. That is a risky change best left for v5 so I don't want to mix in here since this PR is likely to go back to v4.0 as bug fixes. IMO we can rely on the cstdarg in C++ code, but we still have to care a little about the mangled situation in C for the last bits in lib/.

Other bits:

FYI: we should avoid "<" and ">" characters in commit and RR titles since those lines get injected as HTML text on the squid-cache website change listings. I've see a few similar title get badly mangled by the HTML-tag removal scripts and have not found a way to fix that properly yet.

[rousskov]

those macro definitions [...] modify things inside the system headers on some older OS so that the code Squid does contain actually works

Can you give an example of an older OS that needs Squid to provide those VA_ macros?

FWIW, I see no evidence of OS usage of those macros in Squid commit history (I looked at commits f04e118, 68b378b, and 0897af2 as well as Squid bug 20). All I see is a set of Squid-specific macros (for in-house snprintf() implementation) and (unrelated to this topic) #include order dependency problems.

I certainly do not insist on removing those macros, especially if there is evidence that they are used by some OSes. If you know they are needed, then please consider adding a brief comment to document why they are there so that this question does not resurface again. For example:

// These macros are needed for system headers on some old OSes (e.g., XXX).

[rousskov]

avoid "<" and ">" characters in commit and RR titles since those lines get injected as HTML text on the squid-cache website change listings

Noted. Do you think we should add a Squid-specific check to the merge bot to work around these injection bugs? If yes, any other title characters the bot should flag as unacceptable?

[rousskov]

The focus and core of this PR is the removal of the VA_COPY macro from src/*.cc files.

Agreed.

Your updated text appears to moved focus [...] onto the configure.ac change which is the least significant part.

I can see how one can interpret my PR title that way. It was not intentional. Will change to "Use va_copy() on all platforms" after posting this message. FWIW, I wanted to use "unconditionally" but that is misleading because va_copy() use is conditional in some cases. Again, feel free to polish further.

I simply seek to know why such a significant re-write was needed?

IIRC, I decided to polish the original proposed commit message for several reasons, in rough discovery/priority order:

1. The body violated commit message formatting rules.
2. The body renderings included a mangled quote attributed to me that partially duplicated the primary body text and had several other out-of-context issues.
3. The body was missing relevant/useful references to the related bug reports.
4. The title unnecessarily relied on reader's intimate knowledge of Squid internals.
5. The title did not make the effect of removing "hacks" clear.
6. Using C++11 as the title prefix felt awkward and possibly misleading: It could have been misinterpreted as limiting changes to code using C++11 concepts or as implying that the removed code was conflicting with C++11 concepts.
7. The body claimed to fix an unidentified but potentially very important bug.

Please note that there is no reason to dispute or discuss the above. I am just documenting my state of mind that led to the change (because you asked me why I changed the commit message). Some of the above statements could be wrong (or appear to be wrong to you), but it does not really matter in this case.

When polishing the proposed commit message, among other things, I tried to fix all of the above except number 7. For number 7, I asked you to supply the missing info -- I could not guess what the bug was based on the available information. I researched that bug a bit now, and I know understand why StoreEntry::vappendf() was not calling va_copy(), but I need more time (or help) to understand why StoreEntry::vappendf() must always call va_copy(). I do understand why MemBuf needs to call va_copy() after calling printf() or some such. The outcome of this investigation would be more text in the commit message (and possibly title).

P.S. I have already documented what the commit message should contain IMO, but I did not know that when I started to change the message. It took some time to develop that understanding.

[yadij]

Okay, the newest title looks better. I have added mention of the store.cc issue to the description as well now.

Some response to the issues you found (see this as clarification of my views, more than rebuttal):

(1) The body violated commit message formatting rules.

The only "rule" I ever recall discussing amongst developers was how line-length on the titles needing to be less than 72 characters, and keeping the body similar to avoid mangling in the emailed copies. Unfortunately github appears to reformats text at times and mangling things, which leads to your (2) item.

If anubis bot is going to continue checking for and rejecting based on other things, then I think we need a formal policy proposed about them.

If it is colliding with github automatic (re)formatting of texts, that is an issue we need resolved before anubis can be fully utilized.

(2) The body renderings included a mangled quote attributed to me that partially duplicated the primary body text and had several other out-of-context issues.

Sorry about the mangling, I copy-n-pasted what you wrote in the bugzilla comment since that was the initial driver behind this change. It didn't look mangled at the time.

(4) The title unnecessarily relied on reader's intimate knowledge of Squid internals.

Hmm, target audience matter there. I think people reading the commit titles or changelog where they get listed are the dev people who either do have enough of that knowledge or the relevant skill to lookup what it means if they are interested.

(5) The title did not make the effect of removing "hacks" clear.

That is definitely not the place of a title. That is what description is for, and yes maybe the description was lacking a bit.

(6) Using C++11 as the title prefix felt awkward and possibly misleading: It could have been misinterpreted as limiting changes to code using C++11 concepts or as implying that the removed code was conflicting with C++11 concepts.

Uhm, this PR is making the code rely on a guarantee that only exists in the C++11 cstdarg header. It is relevant to anyone interested in the changes made in v3.4+ due to C++11 so I use this prefix where possible group it in the maintainer changesets that require C++11 compilers.

[rousskov]

If anubis bot is going to continue checking for and rejecting based on other things, then I think we need a formal policy proposed about them.

Agreed.

Right now, Anubis only checks for the well-known 72 character limit, as formalized in Anubis README (search for "72"). I have not seen GitHub screwing that up yet, but YMMV (and your recent 300+ characters/line addition to this PR description clearly violates that limit just like most of your recent master commits do).

In the future, I hope that somebody teaches Anubis to safely reformat for us. GitHub might even help with that, but I have not research/thought about it enough to offer a specific blueprint.

[rousskov]

I adjusted the bug fix description to fix formatting and spelling problems. I also detailed the bug scope a little. As always, please adjust this further if needed.

[rousskov]

The current PR description/future commit message looks good to me. Assuming it does not require significant modifications on your side, I am re-approving this PR.