CRM-19013 - Smarty - Do not truncate compiled templates

[mfb]

Truncating a compiled template leads to concurrency issues if another process is including the same file. See also CRM-11189 for a description of the "bus error" encountered when one of the concurrent processes dies.

---

• CRM-19013: Compiled templates are truncated
• CRM-11189: When CiviMail renders smarty templates for mailing, it saves them to disk

[totten]

This isn't an issue with your patch, but a surprising observation about this section of code -- it looks like validateCompilePath() ("do check can smarty create a file w/ given path" [sic]) would effectively disable caching. If I'm reading correctly, every call to Smarty::fetch() calls Smarty::_get_compile_path(); as long as the resource uses string: notation, it will then call Smarty::validateCompilePath() which then @fopens and @unlinks the file... which means that cache file is always deleted before you get a chance to use it.

(Hrm... this whole scenario for caching string: seems overwrought... as in... it's not sure what to name the cache-file, so it does @fopen+@fclose+@unlink to see if the filename produces a syntax error; and if there is an error, then it makes a uniqueish-but-not-reusable name with time()+rand(). Wouldn't it be simpler to have a more robust naming pattern like $this->compile_dir . sha1($resource_name) . '.php'?)

[mfb]

That does sound more robust (I have no idea why it works this way :) Oh and the other weird thing here is that it opens the file for writing. And for a short period of time this compiled template is now on disk, completely empty, if it didn't already exist. That's not safe for concurrent processes either. For reference this was added in https://issues.civicrm.org/jira/browse/CRM-5890

[mfb]

It looks like even if you remove these lines, templates are still recompiled (at least for mailings), because $smarty->compile_check is enabled.

[tttp]

Oh my,
I've hit that bug too (php 7).
So it's a huge pile of weird, from how the template file names are managed to "why are we even using that for civimail?"

The problem I have needs to run two cronjobs (needed if you want to have meaningful AB tests), but, for some reasons, it does not happen if the mailings are small (ie. it needs to run long enough)

So beside code review, not sure how to reliably test. Any suggestion?

I'm thinking of either going the custom cache road http://www.smarty.net/docs/en/caching.custom.tpl (so we can see better what is happening... or skip the caching altogether for civimail)

... or consider it fubar and jump into flexmail.

[totten]

...not sure how to reliably test...

This.

I think if a couple folks like @mfb and @tttp have been using it in production for a while, then that's the best we can really do. Have you guys been using it?

[tttp]

Yes we have been using it since a month ago in production, and sent 5M emails since the change.

@totten on our side: does work
@mfb did you try it?

[mfb]

Yes we have to use this in production, otherwise we get the dreaded "bus error" and the process dies. This resolves that issue, but occasionally (haven't checked stats, but it's rare) an individual mailing message is empty and CiviCRM therefore won't send it - this is a related concurrency issue due to template files being deleted and empty template files being created by multiple processes sharing the same templates.

[mfb]

...This wasn't getting any traction, so I tried reworking it into something more robust: Using SHA-256 hash as the filename (rather than CRC-32 plus the string itself, which could be too long to be a valid filename, thus the weird creating and unlinking of compiled templates).

[seamuslee001]

hmm I certainly haven't hit this before @totten This makes sense i think to me Tim, its also evident its in production in 2 separate sites and no errors found I think it should be merged on the basis of probabilities that its an improvement

[mfb]

The current version of this PR might only be in use on one site, since I completely rewrote it a week ago. Would be great to get a bit more testing. Maybe it's possible to run the test suite on a civicrm-packages PR?

[seamuslee001]

@mfb i have just checked it out onto a local buildkit build of master and now going to try and run the unit tests on it locally. tho @totten maybe able to get it to run on Jenkins

[seamuslee001]

@mfb i have just kicked off a Jenkins run based on this PR

[seamuslee001]

@totten @mfb unit tests passed on pr-158-1

[mfb]

great! I can't think of a possible regression since the code is mostly just simplified, but if there are any more volunteers willing to test that couldn't hurt..

[mfb]

We've been using this in production so I think it's ready to be scheduled for next release.

[seamuslee001]

@totten I would be a +1 on merging this, it has passed unit tests and appears to have been in production in one environment at least so i think it would be fairly stable

[mfb]

I was looking at how Smarty generates keys for cached templates these days - looks like it uses sha1. However that was implemented some time ago, and given the SHAttered attack and general migration from sha1, sha256 seems reasonable to me.

[xurizaemon]

Would SHA1 vs SHA256 make any difference? AFAICT we only use a small chunk of the hash anyway and they need to be predictable for use as a cache key. If we're seeing duplicate attempts to open the same file simultaneously, it's not the hash that's at fault MO.

(Not 100% but at a glance it seems like Smarty is using CRC32?)

I've wondered about speed-testing $smarty->setCompileDir('/dev/null'); against compile to disk, and I believe there is functionality or patches around to put templates_c in Redis etc. (Should also nicely address things like CRM-15632.)

[mfb]

I used sha256 given that sha1 is generally deprecated (Twig and Drupal 8 for example use sha256)

[seamuslee001]

Jenkins test this please

[mfb]

We decided to disable CIVICRM_MAIL_SMARTY - doing so works around most instances of this bug (unless your site is really busy, I guess). I wasn't planning to keep maintaining this PR since it's not critical for our instance, but this code is still "wrong" and should be fixed.. :)

[mfb]

EFF folks are interested in re-enabling Smarty so that they can add more complex logic to email templates. So there's a good chance I may resume work on this PR.

[tttp]

Have you considered using a more modern template language? Eg mustache or twig?

…

On Tue, 13 Aug 2019, 21:55 mark burdett, ***@***.***> wrote: EFF folks are interested in re-enabling Smarty so that they can add more complex logic to email templates. So there's a good chance I may resume work on this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#158?email_source=notifications&email_token=AAA7LKV5F7EYFXD4BAVYEETQEMGRPA5CNFSM4CH4KAF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4GZNEQ#issuecomment-520984210>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA7LKU42SPEFXFRDTP7ASTQEMGRPANCNFSM4CH4KAFQ> .