[pmg-devel] code adjustment for PMG in /usr/share/perl5/PMG/DBTools.pm

[Thomas Lamprecht]

On 4/13/19 10:32 PM, Stoiko Ivanov wrote:
> Hi,
> 
> Patrick, thanks for picking this up and bringing it to our attention!
> On Fri, Apr 12, 2019 at 12:35:01PM -0400, Patrick Fogarty wrote:
>> Thomas
>>
>> I agree that my change works, but introduces the potential for problems, and
>> therefore may not be the "correct" answer. In fact I have no confirmation
>> that everything is okay. Everything appears to be okay after upgrade, except
>> that tracking center loops forever with a "connection error". However I did
>> not press matters to determine if there truly is an issue.
> 
> Hmm - this should not happen - but is probably not directly related to the
> postgres_admin_cmd issue. However I could imagine that some services did
> not get setup correctly, because of the failing postinst script - You could
> try to run `dpkg --configure -a` (with your patch in place) and restart
> all services (or the whole PMG-instance).
> 
> If this does not fix the issue - I would suggest to open a bug-report in
> https://bugzilla.proxmox.com (or a forum thread) for further analysis.
> 
>>
>> The environment...
>> PVE: 5.3-12
>> PMG: 5.1-3
>>
>> The is a test environment, so we are using your community repo, not
>> subscription. However it is happening in subscription as well.
>>
>> PMG is running inside a container based on Debian Stretch. It is about as
>> unmodified as you can get.
>>
>> We did make modifications to the PMG install. They are limited to
>> /var/lib/pmg/templates/main.cf.in and /var/lib/pmg/templates/init.pre.in
>> (geoip) and some custom spamassassin rules. Beyond that, nothing has been
>> touched.
> 
> If you modify the configuration templates - please put them in
> '/etc/pmg/templates' - that way they are preserved across upgrades (see
> section 4.3 of the admin guide
> https://www.proxmox.com/images/download/pmg/docs/pmg-admin-guide.pdf).
> 
>>
>> The upgrade failure happens at the very end during the configuration phase.
>> As the post mentioned, I traced the issue to PMG:: DBTools::
>> postgres_admin_cmd. But you have already reviewed the post and the code, so
>> you already know that.
>>
> Your analysis is correct - and as the strace output of the OP shows the
> setresuid call is successful, but $! (perl's variable for errno still is
> set from a previous syscall (the TCGETS ioctl).
> I managed to reproduce the issue by installing 'nscd' (without it the
> getpwnam call opens and reads '/etc/passwd' directly and this resets errno
> to 0 somewhere, with it installed it asks the nscd, and errno still is
> set).
> 
> We would gladly welcome a patch by you! (I would probably use the approach
> from [0] for readability reasons). Although setting $! to 0 before `$> =
> $uid;` should work as well.
> If you don't find the time, I can also send the patch.

FWIW, both options are ok to me, but I'd like to have the $! = 0; with a comment
here as a reminder for people reading and/or copying this, could be combined with
the "is the euid now the one we wanted" check too (or just that _and_ a comment
would work too for me, but such things need to be recorded).

> 
> BTW. this line is perl's way of 'su postgres' (what Thomas refers to as
> Perl_magic_set :)

Perl_magic_set is actually the perl's interpreter (or whatever it really is)
method to handle the single letter short hand variables, like $@, $_, $!, $;
... which it calls "magic variables", it is surprisingly readable, even though
multi platform support is handled there too, in a single big switch/case...
So it's not only responsible for "su" but actually for a /bit/ more ;-)

> 
> I quickly skimmed through the pmg-api repos (with `git grep '\$\!') - and
> this seems the only problematic use of relying on $! being 0 instead of
> explictly checking for an error-condition.
> 
> cheers,
> stoiko
> 
> [0] https://www.perlmonks.org/bare/?node_id=117878
>>
>>
>> Let me know how I can help further.
>>
>>
>>
>> Thomas Lamprecht wrote on 4/12/19 11:23 AM:
>>> Hi,
>>>
>>> On 4/12/19 4:17 PM, Patrick Fogarty wrote:
>>>> If you follow this thread...
>>>> https://forum.proxmox.com/threads/setuid-postgres-114-failed-inappropriate-ioctl-for-device.41414/
>>>>
>>>> ...you will find that I had an issue, as did others, performing upgrades/updates. I adjusted my code manually and things went back to normal. Last month you released PMG 5.2. The upgrade downloads a package that includes the original code, so the upgrade overwrites my changes, then fails the upgrade process "pmg-api --configure", after which I believe the process is in an incomplete state.
>>>>
>>>> How can I assist, beyond my post in the above thread, showing the code change I made?
>>> Either, you could sent a patch here, for this see:
>>> https://pve.proxmox.com/wiki/Developer_Documentation
>>> (while written for PVE, almost all is valid for PMG too)
>>>
>>> Else you normally also can make a Bug report, which does not gets as easily forgotten as
>>> some Forum/Mailing list thread: https://bugzilla.proxmox.com/enter_bug.cgi
>>>
>>> But posting here may be enough this time, Stoiko, could you please take a look
>>> at the proposed change:
>>> https://forum.proxmox.com/threads/setuid-postgres-114-failed-inappropriate-ioctl-for-device.41414/#post-233016
>>>
>>> Looking at perl's source code at the Perl_magic_set case for EUID (case '>')
>>> we just see that there's no special error handling, thus the recommendation
>>> to check $! (errno or sys_errlist[errno] depending on context) after the call.
>>>
>>> We do that, and it seems correct, so either we need to reset it to 0 befor the
>>> seteuid call to ensure we do not check an old value or this is correct an the
>>> seteuid really fails in the posted case..
>>>
>>> Patrick, can you post a bit more details about your setup, you said you run it in
>>> PVE, inside a Virtual Machine (VM) or Container (CT)?
>>>
>>> Because your change could just omit error checking completely, and thus not be
>>> the "correct" solution here..
>>>
>>> cheers,
>>> Thomas
>>>
>>>