Carlo Arenas <carenas@xxxxxxxxx> writes: > makes sense, but I still unsure about the two questions I had above: > > * would it make sense to make it run ONLY if both variables are set to > YES or is it enough to use one (most likely the GIT_TEST one) that we > would enable independently in CI? > > the advantage I see of having both variables is that it is even less > likely to even run accidentally in a desktop somewhere and maybe break > that test, and also keeps the meaning I wanted to attach to it with > that ugly looking flag that no one should ever try to enable in their > workstations unless they really know what they are doing. > > The advantage of ONLY having the GIT_TEST one is that it will be > easier to enable in CI and for whoever wants to play with it on their > workstation as well, but might still encourage people trying to make > it work and wasting their time. Those who want to use it in CI would need to be told (or have to figure it out) by the patch that adds either IKNOWWHATIAMDOING or GIT_TEST_WHATEVER, and as long as the patch does its job well enough, I do not see much difference either way. The only possible difference is if we use IKNOWWHATIAMDOING then a special CI job that may run with tweaked /etc/sudoers would run the test in this series *and* the other test we borrowed IKNOWWHATIAMDOING from, which may not necessarily be what we want to do. > * since we have to enable CI for these to be useful, would that be > something to be done in an additional patch as part of this topic > branch and you will only pull the commits before it to maint to avoid > conflicts, or should it be done completely independently as a mini > feature branch that depends on this one that will be pulled to seen > and merged downwards from it? I'd expect that nobody pays attention to GitHub CI runs on maint-2.30 - maint-2.35 branches when I push out. I am hoping that these fixes are built on maint-2.30 _without_ CI integration (i.e. the SUDO tests won't be run due to lack of IKNOWWHATIAMDOING in the CI environment). A single branch, without CI integration, based on maint-2.30 would be prepared. Let's call that cb/path-owner-check-with-sudo topic. It is merged to another branch, based on v2.36.0. Let's call that cb/test-path-owner-check-with-sudo-in-ci. On that latter branch, changes to CI to tweak /etc/sudoers and set IKNOWWHATIAMDOING would be created. That latter branch will percolate down starting at 'seen', through 'next', 'master' and finally to 'maint'. After all that happens to prove the primary topic (sans CI) is sound, the tip of maint-2.30 would be updated by merging it, i.e. cb/path-owner-check-with-sudo, and then the result would be merged to maint-2.31, ..., percolating upwards to maint-2.35. The resulting maint-2.35 may be merged to 'maint' after that but that should become a "already up-to-date" merge, I would expect, because 'maint' by that time would have got cb/path-owner-check-with-sudo as part of merging cb/test-path-owner-check-with-sudo-in-ci already, and the merge of cb/path-owner-check-with-sudo is the only thing 'maint-2.35' has that hasn't merged to 'maint' at that point. > true, but the apparent check for ULONG_MAX (which should have a > comment added) was really a check not to overflow when assigning the > value we got into uid_t, by sacrificing an unlikely to be valid > ULONG_MAX as an uid. Are you worried about uid_t wider than ulong? strtoul() with !errno test covers the case, doesn't it? SUDO_UID cannot have any integer that cannot be represented in uid_t and if strtoul() does not say ERANGE, we know whatever value in SUID_UID did not overflow ulong. > it ALSO avoids someone trying to sneak a value that would overflow in > one of the most common configurations we will run where sizeof(long) > > sizeof(uid_t) && MIN_UID_T >=0, by using an equivalent to MAX_UID_T Sorry, -ECANNOTPARSE. If strtoul() can parse everything in uid_t then where is the room for overflowing? We are trying to protect an unsuspecting user who temporary has become 'root' via sudo, and not somebody deliberately hurt themselves or others by setting SUDO_UID deliberately to strange values (once you are 'root', you have easier ways to hurt other people).
