> Just a minor nitpick, but I noticed a typo in a comment in the patch itself: > > ...actual configue file... This is part of the upstream patch, which I copied verbatim. It’s not my typo to correct. > In a fresh lxc container I ran the commands verbatim as given in the SRU bug > report, on both focal and eoan, and got the error message: > > root@triage-focal:/home/bryce# systemctl restart memcached > root@triage-focal:/home/bryce# memcping --servers=127.0.0.1 --binary > --username=foo --password=bar > Failed to ping 127.0.0.1:11211 UNKNOWN READ FAILURE Ah, I had neglected to transcribe the test case step that actually enables SASL authentication: echo '-S' >> /etc/memcached.conf Fixed in the report, and verified in lxc. > I wonder if the patch and test case may deserve to be expanded on a bit. As I > understand it, normally the config file should be stored in a file named > /etc/sasl2/memcached.conf, but the problem is that due to a bug > /etc/sasl2/memcached.conf/ may have inadvertently been created as a > subdirectory, and memcached.conf placed inside of that. So the purpose of the > patch is to make memcached check the incorrect location just in case. Is that > understanding correct? No. The bug is that, no matter what state the actual filesystem is in, memcached *looks* for the incorrect /etc/sasl2/memcached.conf/memcached.conf path by default. There is no reason this path would exist, unless the admin has intentionally created it to work around this exact bug. > Another thing I notice in the patch is that it is also checking for if the > config file was copied to /etc/sasl or /etc/sasl2. I.e. where those are > supposed to be directories they've been created as files. You’re misreading the patch. Note that there are three arrays involved, one of which is old and two of which are added by the patch. The logic is: • If /etc/sasl/memcached.conf/memcached.conf is a file, then pass the /etc/sasl/memcached.conf directory to the SASL library. • If /etc/sasl/memcached.conf is a file, then pass the /etc/sasl directory to the SASL library. • If /etc/sasl2/memcached.conf/memcached.conf is a file, then pass the /etc/sasl2/memcached.conf directory to the SASL library. • If /etc/sasl2/memcached.conf is a file, then pass the /etc/sasl2 directory to the SASL library. The SASL library expects a directory. A file at /etc/sasl or /etc/sasl2 will not work, has never worked, and is not expected to work. > For the test case in the SRU bug, I notice it is testing the case where the > config file is at /etc/sasl2/memcached.conf, but I wonder if it should also > include steps to test that config stored at the alternate locations also work? Added to the report. > In looking at the patch itself, I notice that the fix only applies when built > with HAVE_SASL_CB_GETCONFPATH defined, and not when HAVE_SASL_CB_GETCONF is > defined. Guessing that which one is defined depends on what sasl library was > built against... can you explain in the patch description where these are > defined? (Perhaps the reason the test cases weren't working for me was > because whatever defines HAVE_SASL_CB_GETCONFPATH isn't being installed?) Keep in mind that the patch description is copied verbatim from upstream along with the patch; I didn’t write either. cyrus-sasl has never defined SASL_CB_GETCONF, and SASL_CB_GETCONFPATH was added in 2006, so for Ubuntu’s purposes we don’t need to worry about any other cases: https://github.com/cyrusimap/cyrus-sasl/commit/ce6608bb400180294184116ab8148a15df3bb2db > However, I get the sense that this isn't the full story. I'm guessing there > is the equivalent of a mkdir -p somewhere, that is using the conf filename > instead of the conf directory. It would be interesting to have an outline of > steps through the code that result in the failure mode. No, as described above, the bug is in where the config file is read, not where it’s written. There’s no mkdir involved. The file is only ever written manually by the admin. > Finally, with the regression potential section of the SRU report, remember not > only to answer the question of "how likely is a regression" but also "if a > regression did occur, what would we expect it to look like?" Added to the report.