Confirmed in git. I'll look into it soon.

Just in case this helps....

Adding in some debug statements suggests to me that this bug is contained below this code in on_user_settings_clicked() defined in src/users/callbacks.c

                if (gst_dialog_is_authenticated (tool->main_dialog)) {
                        /* change users/groups configuration */
                        oobs_object_commit (GST_USERS_TOOL (tool)->users_config);
                        oobs_object_commit (GST_USERS_TOOL (tool)->groups_config);
                } else {

Specifically, the full_name field appears to be correct at this point but somewhere in the commit of users_config the apostrophe causes a problem.  I'll see if I can work out a bit more, but I'm not sure if we're outside the realm of GST itself at this point.

PS: I can verify that this bug exists in the code in use on ubuntu Karmic, which is GST vs 2.27.2-0ubuntu2.

If the name is valid until the commit, the bug must be in the system-tools-backends, or in liboobs. Since liboobs is merely a wrapper working via D-Bus, I suspect the s-t-b. They're written in perl, which will require you to master the basics of that language. Not sure I can help, but please ask if you need more info.

Yes, I gathered that and am knee-deep in liboobs now.  

STB, now that's helpful, thanks.  I'll have a look.  I do have a reasonable grasp of Perl (better than my C, anyway!).

Thanks for the pointers.

The patch below fixes it for me.  The trouble is that a shell command is being constructed to do
  usermod -c 'Sean O'Reilly' seanor

which naturally doesn't work as the apostrophe in the middle breaks things.  I've changed it to
  usermod -c "Sean O'Reilly" seanor

which will also break, where there's a " in the name, though that seems like it should be less common.  This fix worked for me initially but I've been messing around since and can't seem to get it to work now.  I'm wondering if there's some strange code caching mechanism involved here that I don't know about or something but my changes to the perl code don't always appear to have the expected effect.



gavinmc@teenie:~$ diff -u /usr/share/system-tools-backends-2.0/scripts/Users/Users.pm /tmp/Users.pm 
--- /usr/share/system-tools-backends-2.0/scripts/Users/Users.pm 2009-08-17 01:03:01.000000000 +0100
+++ /tmp/Users.pm       2009-08-17 01:03:46.000000000 +0100
@@ -478,7 +478,7 @@
   }
   else
   {
-    $command = "$cmd_usermod -c \'" . $str . "\' " . $login;
+    $command = "$cmd_usermod -c \"" . $str . "\" " . $login;
   }
 
   &Utils::File::run ($command);

Nice catch! I have no commit rights to the s-t-b right now, but I hope I'll get them soon.

A possibly better fix would be to detect ' and " characters, and escape them using \ in front of them. I guess that's not too hard to do. Then we could possibly detect more shell special chars, but maybe we don't care about them (|< and friends) and should refuse them before they reach that line. Though I don't see why we should prevent people from using any char they want, be it to draw ASCII art in their names.

About the problems around reproducing the fix: be sure you stop and restart the s-t-b from /etc/init.d, but I can't find any other reason why it shouldn't work.

(In reply to comment #6)
> Nice catch! I have no commit rights to the s-t-b right now, but I hope I'll get
> them soon.

Well, I guess if I add a simple patch to this bug, hopefully someone kind will come along and review/apply it. 

> A possibly better fix would be to detect ' and " characters, and escape them
> using \ in front of them. I guess that's not too hard to do. 

That's exactly what I was going to propose, but haven't been able to get anything working.  I'll hopefully send on a patch this evening.

> Then we could
> possibly detect more shell special chars, but maybe we don't care about them
> (|< and friends) and should refuse them before they reach that line. Though I
> don't see why we should prevent people from using any char they want, be it to
> draw ASCII art in their names.

I don't immediately see a problem with people having special chars in their fullname. Although I suppose it's possible some obscure program which reads in the comment from /etc/passwd might choke on something.

> About the problems around reproducing the fix: be sure you stop and restart the
> s-t-b from /etc/init.d, but I can't find any other reason why it shouldn't
> work.

Ah!  That might help then.  I'm not used to delving in the depths of GST and STB like this.  I don't see STB among the init scripts on Ubuntu.  Any idea where I should be looking?

Thanks for your help,
Gavin

(In reply to comment #7)
> Well, I guess if I add a simple patch to this bug, hopefully someone kind will
> come along and review/apply it.
Don't expect that... ;-) The s-t-b are loosely maintained by Carlos Garnacho, but he never reads GNOME bug reports AFAIK. Last release is from April and only included updated translations. But I'll take care of that.

> > A possibly better fix would be to detect ' and " characters, and escape them
> > using \ in front of them. I guess that's not too hard to do. 
> 
> That's exactly what I was going to propose, but haven't been able to get
> anything working.  I'll hopefully send on a patch this evening.
Good.

> I don't immediately see a problem with people having special chars in their
> fullname. Although I suppose it's possible some obscure program which reads in
> the comment from /etc/passwd might choke on something.
If some program doesn't handle that, it's not our problem. Users that want to use special chars will report bugs again those buggy tools if they exist. The only char we should refuse is ',' since it's used to separate fields. Maybe the code already checks for that.

> Ah!  That might help then.  I'm not used to delving in the depths of GST and
> STB like this.  I don't see STB among the init scripts on Ubuntu.  Any idea
> where I should be looking?
Oh, using Karmic. I guess it's been moved to D-Bus activation, which means it's no longer started on boot (that was stupid), but when it's called. Though a bug prevents it from being stopped when done. I guess you can stop it easily playing with ps and kill, anyway something like 'killall perl' will do.

Hi,

(In reply to comment #8)

> Don't expect that... ;-) The s-t-b are loosely maintained by Carlos Garnacho,
> but he never reads GNOME bug reports AFAIK. Last release is from April and only
> included updated translations. But I'll take care of that.

Fair enough.

> If some program doesn't handle that, it's not our problem. Users that want to
> use special chars will report bugs again those buggy tools if they exist. 

Of course.

> The only char we should refuse is ',' since it's used to separate fields. 
> Maybe the code already checks for that.

That and ":" and "\n" which are all delimiters in /etc/passwd.  I know : is caught up at users-admin tool where an error can be returned.

> Oh, using Karmic. I guess it's been moved to D-Bus activation, which means it's
> no longer started on boot (that was stupid), but when it's called. Though a bug
> prevents it from being stopped when done. I guess you can stop it easily
> playing with ps and kill, anyway something like 'killall perl' will do.

I see.  I'll look this evening, thanks.

Gavin

Created attachment 141005 [details] [review]
patch to fix issue with apostrophes

This seems to sort out this issue, escaping any apostrophes in the comment and thereby allowing apostrophes in the fullname.

One note: This will not allow you to place certain a backslash in the full name, as the backslash is a special character even within the ''.  I guess this shouldn't be too much of an issue?

Not sure why you think we should escape blackslash at all. I don't really understand the line you're adding as I'm not very good at regular expressions and perl... :-) Do you mean that the way you escape apostrophes necessarily prevents form using \ too? If it's the case, that's not really a problem to me: if you really want it in your Real Name, just edit /etc/passwd.

Would you mind formatting your patch using 'git format-patch', adding a more explicit title and explanation? Then I'll commit it to system-tools-backends-clone on git.gnome.org, and Carlos Garnacho will pick the commits from there.

Hi,


(In reply to comment #11)
> Not sure why you think we should escape blackslash at all. I don't really
> understand the line you're adding as I'm not very good at regular expressions
> and perl... :-) 

by way of explanation, this line:

  $str =~ s/'/\'/g;

means:

1. take $str and apply the following expression to it ($str =~)
2. the expression is substitution (s) of the 1-char string ' (s/'/) with the 
   2-char string \' (s/'/\'/).
3. The trailing g means do this for all matching instances in $str.

In other words, escape any apostrophes in $str with a backslash.

> Do you mean that the way you escape apostrophes necessarily
> prevents form using \ too? If it's the case, that's not really a problem to me:
> if you really want it in your Real Name, just edit /etc/passwd.

What I mean is that if a user tries to put a backslash in their name this may cause issues.  You were suggesting above that ascii art in names might be something someone might do, so I thought I'd point out what chars might still be an issue.  I've never heard of a backslash in a name, but most countries wouldn't have apostrophes in names either.

> Would you mind formatting your patch using 'git format-patch', adding a more
> explicit title and explanation? Then I'll commit it to
> system-tools-backends-clone on git.gnome.org, and Carlos Garnacho will pick the
> commits from there.

I haven't been working from the git repo (I'm just using the source code in the ubuntu repos), but I guess I can download the tree tonight and do this this evening.  Is there a page describing gnome's git tree location?

OK, thanks for the explanation. Regarding blackslashes, I've checked that using them in Real Names seems to be working, so I don't see any reason to remove them (not that I'm really supporting the ASCII art option...).

If you don't have the git repo set up now, I can do the commit for you, that's not really a big work. Though if you want to get it for later work (I hope you'll continue to help us ;-) ), see http://live.gnome.org/Git/Developers, which is a very good entry point.

Cheers, I'll commit you patch soon.

(In reply to comment #13)
> OK, thanks for the explanation. Regarding blackslashes, I've checked that using
> them in Real Names seems to be working, so I don't see any reason to remove
> them (not that I'm really supporting the ASCII art option...).

I haven't checked but I think \ may only have meaning where it's in front of a special char.  I _think_ you should have problems inserting \', but we really are headed into the realms of ascii art there.

gavinmc@ceartgoleor:~$ echo 'a\d\ d'
a\d\ d
gavinmc@ceartgoleor:~$ echo 'a\d\' d'
> 

> If you don't have the git repo set up now, I can do the commit for you, that's
> not really a big work. 

I appreciate that, thanks.  Please stick my email address on the patch in case anything goes wrong.

> Though if you want to get it for later work (I hope
> you'll continue to help us ;-) ), see http://live.gnome.org/Git/Developers,
> which is a very good entry point.

I have another more substantial issue in users-admin I'd like to look at so this is very helpful.  Thanks!

Thanks for your work Gavin. Saves me from doing it (I was only up to the "poking around in liboobs step).

Some comments:
1) According to "man chfn", these fields shouldn't have any control characters, commas, colons or equals signs. It might be worth either stripping these out in s-t-b, or asserting something there.

2) If the patch was modified slightly to first escape backslashes:

$str =~ s/\\/\\\\/g;
$str =~ s/'/\'/g;

Then I'd probably rest a bit easier. Be aware that the code above is completely untested... the intention of it is to replace a "\" with a "\\", before replacing a "'" with a "\'"

1) So we need to escape those too, and check that the gst also refuse them, so that we don't fail without explanation.

2) I leave you two debate around that. ;-)

If you could provide me with a patch tomorrow, I'd be glad to commit it, I'd like to make a release just after that so that the gst can depend on PolKit1.

Hi,

(In reply to comment #15)
> Thanks for your work Gavin. Saves me from doing it (I was only up to the
> "poking around in liboobs step).

We all have to try and pull our weight in the community.  I'm not a very regular bug fixer but it's nice to help out.

> 1) According to "man chfn", these fields shouldn't have any control characters,
> commas, colons or equals signs. It might be worth either stripping these out in
> s-t-b, or asserting something there.

That sounds reasonable.  What exactly do you think is meant by "no control characters".  I wonder is backslash one.  This would probably make [2] moot.

> 2) If the patch was modified slightly to first escape backslashes:
> 
> $str =~ s/\\/\\\\/g;
> $str =~ s/'/\'/g;
> 
> Then I'd probably rest a bit easier. Be aware that the code above is completely
> untested... the intention of it is to replace a "\" with a "\\", before
> replacing a "'" with a "\'"

I need to check but it seems that within apostrophes, the backslash only has meaning where it is escaping an apostrophe so it may not need escaping.  Some experimentation required.  Also, I don't see that there can be a security issue as users-admin runs as the user in question anyway and they could just as easily run chfn directly.

(In reply to comment #17)
> 
> That sounds reasonable.  What exactly do you think is meant by "no control
> characters".  I wonder is backslash one.  This would probably make [2] moot.
> 

Control characters are characters with ascii value < 32. None of them are printable, so backslash is not one. 

> I need to check but it seems that within apostrophes, the backslash only has
> meaning where it is escaping an apostrophe so it may not need escaping.  Some
> experimentation required.  Also, I don't see that there can be a security issue
> as users-admin runs as the user in question anyway and they could just as
> easily run chfn directly.

My understanding of things is that the user might not be root, they may just have authorization thanks to policy kit.

Actually, since this is being executed by a shell, there may even be a privilege escalation bug in here. If I'm a user with policy kit permission to change this stuff, and I change my name to:

Andy'; touch /0wned #

I haven't looked enough at the code to see if the higher levels are preventing this sort of thing... but it should probably be handled at the lowest level.

This function looks like it will fix everything though:

http://perldoc.perl.org/functions/quotemeta.html

Just to be clear... I'm possibly way off. And my quick test of changing my name with the User Settings tool to:

Andy "; zenity --error

Seems to work just fine (with /etc/password recording my name as exactly that). So I don't think there is any need to panic... it would just be good to work out exactly where the escaping is being done, and if necessary, move it down to the lowest possible level (so the hypothetical bad guy can't just write their own tool that uses the normal policykit stuff, but doesn't escape the characters).

Also, I believe that "" quotes behave differently to '' quotes in the shell (though it would also be nice to verify exactly which shell perl uses). For example:

echo ' \' '

expects another '
and:

echo " \" "

prints a space, then a " then another space.

quotemeta does not seem to do what we want, since it will escape non-word characters. We sheerly want to remove some of them, and we apparently only need to escape ' and " so that they don't get lost, if needed. Is it possible to remove "control characters, commas, colons or equals signs" as you said?

Though, not security issue as been reported about that, so that may not be so vulnerable as we think. I'm more concerned about getting a malformed /etc/passwd file than running arbitrary commands as root, which seems to fail right now.

(In reply to comment #18)

> > I need to check but it seems that within apostrophes, the backslash only has
> > meaning where it is escaping an apostrophe so it may not need escaping.  Some
> > experimentation required.  Also, I don't see that there can be a security issue
> > as users-admin runs as the user in question anyway and they could just as
> > easily run chfn directly.
> 
> My understanding of things is that the user might not be root, they may just
> have authorization thanks to policy kit.
> 
> Actually, since this is being executed by a shell, there may even be a
> privilege escalation bug in here. If I'm a user with policy kit permission to
> change this stuff, and I change my name to:

I could be wrong here but I had assumed that when the tool called "chfn" for a given user, it called it _as_ that user so a privilege escalation was not on the cards.  Ordinary users can run chfn on their own account, just not those of others (hence the "unlock" dialog.

If this isn't the case and the tools itself is effectively setuid, then this has also sorts of worrying possibilities.

Do those perl scripts always runs as root or would they run as "gavin" where users-admin is running strictly as gavin?

(In reply to comment #20)
> quotemeta does not seem to do what we want, since it will escape non-word
> characters. We sheerly want to remove some of them, and we apparently only need
> to escape ' and " so that they don't get lost, if needed. Is it possible to
> remove "control characters, commas, colons or equals signs" as you said?

I don't even think we need to escape ".  The perl code constructs a command like:

  usermod -c '<string>' seanor

The use of apostrophes means that very few chars in <string> will have special meaning.  
  " definitely doesn't (I'm pretty certain)
  ' obviously does.
  \ doesn't, I think, except if it's \'

> Though, not security issue as been reported about that, so that may not be so
> vulnerable as we think. I'm more concerned about getting a malformed
> /etc/passwd file than running arbitrary commands as root, which seems to fail
> right now.

Unless there's really a setuid (ie the user runs processes as root without a sudo), I can't see how this can be a security issue, but I don't know the code well enough to know that.  

Gavin

The backends run setuid, and so does chfn. The reason is, /etc/passwd can't be written without the required rights.

I've tried to commit the first simplest patches, but they don't seem to work here. Let's say they won't go in the next release (today), but a little later, so that the patch is The Right One.

(In reply to comment #20)
> quotemeta does not seem to do what we want, since it will escape non-word
> characters. We sheerly want to remove some of them, and we apparently only need
> to escape ' and " so that they don't get lost, if needed. Is it possible to
> remove "control characters, commas, colons or equals signs" as you said?

Sorry, I was talking about escaping for the shell.

At the moment it looks something like:

usermod -c "Andy Owen,,,," aowen

Where the "Andy Owen,,,," is created by just joining some strings together with ',' to separate them.

I was thinking (but not communicating at all) that perhaps it could be written:

usermod -c Andy\ Owen,,,, aowen

With quotemeta doing the escaping.

> 
> Though, not security issue as been reported about that, so that may not be so
> vulnerable as we think. I'm more concerned about getting a malformed
> /etc/passwd file than running arbitrary commands as root, which seems to fail
> right now.

Yeah, I suspect that usermod is smart enough not to let us put bad characters into the comment field, then we should be right.

According to "man dash":

> Enclosing characters within double quotes preserves the literal meaning
> of all characters except dollarsign ($), backquote (‘), and backslash
> (\).  The backslash inside double quotes is historically weird, and
> serves to quote only the following characters:
>       $ ‘ " \ <newline>.
> Otherwise it remains literal.

(and single quotes aren't appropriate for this, as they don't allow any escape characters, so a name with a ' is impossible)

(In reply to comment #22)
> I've tried to commit the first simplest patches, but they don't seem to work
> here. Let's say they won't go in the next release (today), but a little later,
> so that the patch is The Right One.

This is a little embarrassing.  I somehow convinced myself that patch worked but it didn't. Reading a little further, while you can escape apostrophes in that way within some languages (like perl), you can't in the shell.

I've tried out Andy's quotemeta() solution and that seems to work pretty well.  I'm attaching a new proposed patch based on that, though it's not really my patch :-)

Created attachment 141197 [details] [review]
Patch #2 to fix apostrophes within the chfn command in Users.pm 

This is Andy's idea and seems in my tests to solve the issues with apostrophes.

(In reply to comment #23)

> I was thinking (but not communicating at all) that perhaps it could be written:
> 
> usermod -c Andy\ Owen,,,, aowen
> 
> With quotemeta doing the escaping.

Actually, in my tests, the quoted string looks a little more complex:

/usr/sbin/usermod -c Abel\ O\'Leary\,\,\,\, abel

but that seems to work fine.

It's absolutely your patch - you did the hard work :)

I was originally imagining calling quotemeta on all the separate fields, then joining them together (so the commas wouldn't be escaped). But on reading the manpage for dash:

> A backslash preserves the literal meaning of the following character,
> with the exception of 〈newline〉.  A backslash preceding a 〈newline〉 is
> treated as a line continuation.

I would agree that what you are doing by escaping everything is correct and simpler.

I'll have a look at that in a few days. But thanks again for the patch!

(In reply to comment #28)
> I'll have a look at that in a few days. But thanks again for the patch!

Thanks.  I had presumed up to now that this bug was in very recent versions only and I was seeing it as I as running Ubuntu Karmic. However, trying it on my Ubuntu Jaunty desktop at work I see it definitely affects Ubuntu Jaunty (2.6.0-2ubuntu8) and Ubuntu Hardy (2.6.0-0ubuntu7) too.

I've also noticed that the same bug is in the FreeBSD code.  This might as well be fixed while we're at it, but the trouble is I don't have a box to test that on.  I'll attach a proposed patch which fixes both, but ideally, I think we need someone with FreeBSD to test it.

Created attachment 141226 [details] [review]
Suggested Patch v3 to use quotemeta() instread of placing comment in apostrophes

This patch fixes both the FreeBSD and Linux parts of the code.  However, the FreeBSD code is UNTESTED!!

This is based on the code in the Ubuntu Jaunty release and it looks like it probably won't apply to the git tree (the line numbers are offset a bit).  I'll add a patch that applies against the git tree this evening.

Created attachment 141303 [details] [review]
patch to fix apostrophes in fullname in users-admin 

Same patch as before but against the latest git tree.

Is the patch above usable?  Is there any chance it could be applied so it goes into Ubuntu before the next freeze?

If need be, to get it moving we could forgo the untested fix for FreeBSD, though I'm fairly confident it should work.

Sorry for the delay. Since I was not planning to release soon, I've not been trying to commit fixes very quickly. But don't worry, I'm not going to let your work rot in Bugzilla - I'm too much in need for contributors! ;-)

I'll certainly try to publish a new version before Ubuntu 9.10 is released, because it's an important distributor to my eyes. Since it's released in October, we still have some time to tackle this problem. They are quite reactive, so I guess they will include the new version without troubles.

The only thing that prevents me from committing your patch is that I think we could apply the fix to a broader level, namely in Utils::file::run() (in Utils/File.pm). I don't see any reason why escaping special chars in our commandlines could hurt, and that would be more consistent, easier to maintain, and could possibly prevent other bugs from appearing. That should be easy to do, but would require some testing. Could you try that? Else I'll try it when I can.

I see what you mean alright.  The only thing is that we'll have to check over every use of Utils::file::run(), otherwise they may be broken.  For example the command in question here was wrapped in apostrophes and those would be escaped and end up in the command line.

I'll have a look.

You're right, but running
$ rgrep File::run .
from the source dir shows that most calls of this function are very basic, something like run("command $argument"), which is not a problem for us. The only case to change would be the one which we want to fix now.

And we would gain some security against interfaces names being commands and allowing privilege escalation. See https://bugs.launchpad.net/ubuntu/+source/system-tools-backends/+bug/190628 for example (though obviously for now you need to be admin to do that, so not very dangerous).

(In reply to comment #35)
> You're right, but running
> $ rgrep File::run .
> from the source dir shows that most calls of this function are very basic,
> something like run("command $argument"), which is not a problem for us. The
> only case to change would be the one which we want to fix now.

I ran the same grep.  It looks to me like you could break a few other commands which themselves insert an apostrophe.  For example del_user()

    if ($cmd_deluser)
    {
      $command = "$cmd_deluser '". $$user[$LOGIN] . "'";
    }

Surely if you escaped that you'd end up with a literal '<username>' getting passed to the deluser command, no?

> And we would gain some security against interfaces names being commands and
> allowing privilege escalation. See
> https://bugs.launchpad.net/ubuntu/+source/system-tools-backends/+bug/190628 for
> example (though obviously for now you need to be admin to do that, so not very
> dangerous).

I certainly agree with the idea alright.

Hmmm.  First trials at this show that it's rather tricky.  You cannot just run quotemeta on $cmd, eg

  my $escaped_cmd = quotemeta($cmd);
  $command = &get_cmd_path ($escaped_cmd);

because the command has a full absolute path which must not be escaped, or you end up with:

\/usr\/sbin\/usermod\ \-c\ \'forename\ surname\,\,\,\,\'\ username

which won't work.  You could potentially break the command up and escape all but the first word but it's possible that a path or something else which should not be escaped  will come up later.  Indeed, the comments above run() actually give such an example.

I think it's going to be pretty complicated to do the escape in the run() sub.

There may be a safer way to achieve all of this.

http://www.perlhowto.com/executing_external_commands

It appears that what one should ideally do is pass an array equivalent to C's ARGV[].  This causes perl to run the command directly, rather than passing it through a shell.  This means you don't really need escaping and things are inherently more secure against such nasty tricks.  

This would involve a change to all of the callers though.

http://www.perl.com/doc/manual/html/pod/perlfunc/system.html

Is system-tools-backends used by anything other than GST??

Maybe I should have been more explicit about the idea I had. Yeah, we have to break the command string we get in run() into separate arguments before escaping it, which can be a little complex; but overall I think the calls that need to be changed are not many, and I can of course help you do to that review.

Glad you found that documentation about the system() call, using arrays to pass arguments. That's definitely the right way to do this. We already have the absolute path to the tool thanks to get_command_path(), so we only need to split the command string on spaces (which is already done in do_get_command_path(), so we could move the code, or duplicate it if it's still needed there). We can still use ' when passing the command string to avoid that splitting. All in all, your patch is going to get bigger than expected, but that will clean our logic a bit.

I still wonder if we could even change run() so that it takes an array instead of a string to split, but it may not be worth the pain.

No, the system-tools-backends are not used by any program other than the gst (that I'm aware of). But that does not change anything for the present patch AFAICS.

Hi,

(In reply to comment #39)
> Glad you found that documentation about the system() call, using arrays to pass
> arguments. That's definitely the right way to do this. We already have the
> absolute path to the tool thanks to get_command_path(), so we only need to
> split the command string on spaces (which is already done in
> do_get_command_path(), so we could move the code, or duplicate it if it's 
> still needed there). We can still use ' when passing the command string to 
> avoid that splitting. 

That sounds kind of messy to implement to me.  We'd then need to code run() so that you split the command on space, except where that space was wrapped in apostrophes, except where that apostrophe is somehow escaped as it's within the name.  I'd much rather the caller was able to just construct the array.

> I still wonder if we could even change run() so that it takes an array instead
> of a string to split, but it may not be worth the pain.

To be honest, this seems to be the right (if not the only) way to go and it's what I have in mind when I say changes to the callers.

> No, the system-tools-backends are not used by any program other than the gst
> (that I'm aware of). But that does not change anything for the present patch
> AFAICS.

That's good news anyway.

Gavin

I've written a basic patch that moves all run() calls to use an array. Now, the difficult part is that when using system() with an array, you can't set STDOUT and STDERR to /dev/null as we did before. I'm not sure about passing $PATH either.

So we may need to manually fork() before calling exec() so that we set STDOUT and STDERR in the meantime. I'd be glad to find an easier way, though.

(In reply to comment #41)
> I've written a basic patch that moves all run() calls to use an array. Now, the
> difficult part is that when using system() with an array, you can't set STDOUT
> and STDERR to /dev/null as we did before. I'm not sure about passing $PATH
> either.
> 
> So we may need to manually fork() before calling exec() so that we set STDOUT
> and STDERR in the meantime. I'd be glad to find an easier way, though.

This is great, thanks.

Sorry I've gone a bit quiet, I've been rather busy of late.

Created attachment 143406 [details] [review]
Bugs 519273 & 346342 - Clean Utils::File::run() logic by using array of arguments

Using system() with a string instead of an array raises various issues when strings passed include special chars. Overall, it's claner and safer to use an array everywhere. This requires us to use our own execution code since system() does not accept setting STDOUT, STDERR, LC_ALL, and starting processes in the background when using an array. Note that run_backticks() still use the old logic, which is safe(r) because those commandlines are static. However, this commit also cleans get_cmd_path() by merging do_get_cmd_path() into it, and removing duplicate settings like $PATH.

Gavin, would you have some time to play with this patch and try to find any bug it could introduce? I'm not very easy with including this patch in a minor release soon, I'd like to be sure it works in all the cases we use it. I've already found that a tiny detail that I missed had broken users creation in a previous version.

Created attachment 143543 [details] [review]
Clean Utils::File::run() logic by using array of arguments

Fixed in 2.8.2, patch above pushed as ee59351bfa31ee60779a20a79b1b49f308a777b0.

(In reply to comment #46)
> Fixed in 2.8.2, patch above pushed as ee59351bfa31ee60779a20a79b1b49f308a777b0.

I must express sincere thanks to Milan for his work on this, both in the debugging, writing the patch, testing, bugfixing and getting his patch applied.  All this while I wasn't even testing his patches.

Absolute full marks to Milan and many sincere thanks for getting this fixed!

Gavin