Hi Daniel, thanks for your thorough review, shame on me !
I relied very much on the test suite - I remembered it as pretty complete, but it didn't catch the wrong code. Improved the test suite to stumble over the bad code and fixed the psl.c accordingly. More comments follow inline... On 02/23/2018 03:51 AM, Daniel Kahn Gillmor wrote: > Hi Tim-- > > Thanks for this new version of libpsl! > > On Thu 2018-02-22 10:31:24 +0100, Tim Rühsen wrote: >> this release comes with a new function flag PSL_TYPE_NO_STAR_RULE to be >> used with psl_is_public_suffix2(). >> It disables the PSL 'prevailing star rule' and thus returns false for >> TLDs that are not explicitly listed in the PSL. >> >> The new flag makes only sense in combination with either PSL_TYPE_ANY, >> PSL_TYPE_ICANN or PSL_TYPE_PRIVATE. > > Layering in this flag on the "type" argument makes the API tougher to > reason about than i was expecting. I'm trying to follow the logic in > the implementation, but i think i might be missing it. > > there are tests in psl.c that compare type directly to PSL_TYPE_ICANN, > without necessarily stripping off any extra flag bits: > > e.g.: > > /* check for correct rule type */ > if (type == PSL_TYPE_ICANN && !(rc & _PSL_FLAG_ICANN)) > goto suffix_no; > else if (type == PSL_TYPE_PRIVATE && !(rc & > _PSL_FLAG_PRIVATE)) > goto suffix_no; > > if (rc & _PSL_FLAG_EXCEPTION) > goto suffix_no; > > > I note that type hasn't been touched at this point (unless > suffix.nlabels == 1, which isn't always the case). These == comparisons > are guaranteed to fail if you treat type as a bitfield and more than one > bit is set :( Am i missing something? > > Come to think of it, i don't know how this would work with PSL_TYPE_ANY > either -- can you explain that to me? You are right, the PSL_TYPE_NO_STAR_RULE flag must be reset before going further in the code. The == comparisons work out since 'type' either is equal to PSL_TYPE_ICANN, PSL_TYPE_PRIVATE or (PSL_TYPE_ICANN|PSL_TYPE_PRIVATE). We assume a rule to be either in the private or in the icann section, not both. With these assumptions, the == comparison makes sense. E.g. if (type == PSL_TYPE_ICANN && !(rc & _PSL_FLAG_ICANN)) means If the lookup is restricted to ICANN and the found rule is not in ICANN section, we return 'not a public suffix'. > Maybe the test suite needs to be augmented to test this more rigorously. > I note that the live PSL has several entries with * rules with multiple > suffix labels, which might be useful to test against. here are three > that might be interesting test cases: > > - *.nom.br > - *.sch.uk > - *.compute.amazonaws.com We, I added *.compute.amazonaws.com tests to test-is-public.c and test-is-public-builtin.c without triggering the issue. But test-is-public-all.c triggered on some exceptions when extending the code. It should now test any combination of flags with all entries in the PSL. What we still don't have is a test for arbitrary domain that are *not* in the PSL. They all should return 0 (except for TLDs when PSL_TYPE_NO_STAR_RULE is not given). Have to think about a practical algorithm to generate and test those. >> 22.02.2018 Release V0.20.0 >> * Remove hard-coded gcc flag in Makefile.am >> * Prevent excessive CPU cycles on large inputs >> * New flag PSL_TYPE_NO_STAR_RULE to skip star rule > > this release appears to update LIBPSL_SO_VERSION like so: > > -AC_SUBST([LIBPSL_SO_VERSION], [7:0:2]) > +AC_SUBST([LIBPSL_SO_VERSION], [8:0:2]) > > This results in libpsl.so.6, when before it was libpsl.so.5. but i > don't see how this makes sense. :/ > > Following the instructions in configure.ac for updating > current:revision:age i don't even see how it's possible to arrive at > 8:0:2. It looks like you took steps 3 and 4, but missed steps 5 and 6: > > 5. If any interfaces have been added since the last public release, then > increment age. > 6. If any existing interfaces have been removed or changed since the last > public release, then set age to 0. > > I wish SONAME management was less confusing! Me too :-( I know we had this issue before and really tried to not mess it up again. It was my intention to *not* bump up SONAME, lol. > If you think that the inclusion of this flag warrants an SONAME bump > (i.e., steps 5 and 6 were triggered), then LIBPSL_SO_VERSION should be > 8:0:0 (resulting in libpsl.so.8, and triggering a rebuild of all tools > against this new package). > > But if you think the changes of this version do not warrant an SONAME > bump (i.e. only step 5 was triggered), then LIBPSL_SO_VERSION should be > 8:0:3 (resulting in sticking with libpsl.so.5, and no recompilation > needed). > > There's a caveat here -- some package could be built against a newer > version of libpsl.so.5, knowing about the flag, and then it could be > transferred to a machine with an older version of libpsl.so.5 that > *doesn't* know about the flag. > > I don't think any older versions of libpsl check whether any of the > unknown bits in type are set (though as you can see above, i don't grok > the logic here very well), which means that older versions might > silently ignore this new flag. If another tool is depending on the flag > having the intended effect, they could get in trouble if they're > transferred to a machine with an older version of libpsl.so.5. But an > soname bump is a pretty heavyweight tool just for adding a flag. > > Let me know what you think we should dohere. I'm reluctant to include > this version in debian without sorting out both of these concerns, but > if you've got a plan for making them make sense, i'd be happy to think > it through with you. You are right with your above concerns... there *might* be cases where someone gets burned (app using new flag linked with old library). We had that before and we fixed it by releasing a version reverting the SONAME bump. Not 100% perfect, and I am not sure how much this affects real-life systems. What's your advice ? Regards, Tim -- You received this message because you are subscribed to the Google Groups "libpsl-bugs" group. To unsubscribe from this group and stop receiving emails from it, send an email to libpsl-bugs+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Description: OpenPGP digital signature