bugGNUstep - Bugs: bug #34490, -[NSWorkspace open*] native system...

Group
 
 

bug #34490: -[NSWorkspace open*] native system integration

Submitter:  julian <julian_>
Submitted:  Wed 05 Oct 2011 09:59:03 PM UTC
Votes: 5
 
Category:  Gui/AppKit Severity:  3 - Normal
Item Group:  Change Request Status:  Ready For Test
Privacy:  Public Assigned to:  FredKiefer
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 30 Jun 2012 09:59:49 AM UTC, comment #25: 

thanks very much for getting the patch in.

to get everything perfect the talked-about changes to NSPerformService() still should be done, because it opens a dialogue if it doesn't succeed.

julian <julian_>
Fri 29 Jun 2012 12:57:40 PM UTC, comment #24: 

I just submitted a change based on your patch. The main differences are that I extracted your code into a new method that gets used by all three places where we need it and that I only allow one default application and use xdg-open as the fallback on non-Windows systems. Ah, and it is even possible to define a default application on Windows.
The user defaults key for this application is GSUnknownFileTool and has been documented as well.

Sorry for taking so long here.

Fred Kiefer <FredKiefer>
Group Member
Thu 28 Jun 2012 09:10:12 PM UTC, comment #23: 

i did incorporate a lot of feedback, thats why the patch is at revision 5.

i did not specifically address something in fred's last post as it didn't make sense to me at all.

the patch as-is only changes behavior as a last fallback anyway, only if the call to openFile/openURL would fail otherwise, so i can't understand any resistance to just merging it. sure some details could be improved but even in the worst form this patch was infinitely better than mainline, where openFile/openURL only work in a hypothetical gnustep desktop environment and otherwise just fail.

moving the code to _openURLWithFallback and calling that openFile/openURL wouldn't change the behavior one bit and i can't see why anything would be gained by using a wrapper. even if something can be gained by using wrappers or changing the implementation the patch should have been merged 6 months ago to get the functionality and improve on the implementation when someone has time. now we have just nothing in mainline, which is the worst case scenario.


>Maybe we should keep the MINGW part commented out,
>as nobody has tested that up to now.


and none will unless you merge it.

julian <julian_>
Thu 28 Jun 2012 12:37:14 PM UTC, comment #22: 

Did you implement a change which addresses Fred's concerns in his last message?  If so, I would really love to get this in.

One thing which has always bugged me is the idea that we need to wrap external applications in order to have them be used by methods in NSWorkspace.  

I know it's been a while, but we're a small team with limited resources.

Please let us know if you've done those changes.

I'm looking forward to hearing back from you.

Gregory John Casamento <gcasa>
Group administrator
Thu 28 Jun 2012 10:22:04 AM UTC, comment #21: 

6 months later the patch is still not in ;-(

julian <julian_>
Sat 28 Jan 2012 02:48:52 AM UTC, comment #20: 

i am a bit puzzled about the resistance since any changes in my patch are only for a fallback where the current code path doesn't work at all. please keep in mind that anything that calls "xdg-open" is a infinite improvement over what you have now, since the current code doesn't do anything useful at all outside a (hypothetical) gnustep-environment.

i'm not sure how anything can be gained here with wrapping the call to ShellExectureW() in an standalone tool...whats the improvement there?

julian <julian_>
Fri 27 Jan 2012 09:58:51 PM UTC, comment #19: 

I also would like to get a solution for this probkem in, but perhaps in a somewhat stripped down version. What about adding a new private method:

-(BOOL)_openURLWithFallback: (NSString*)url

In that method we would have mostly your code, but instead of any array of tools we would expect the user defined value to point to exactly one tool and use xdg-open as the default.
Maybe we should keep the MINGW part commented out, as nobody has tested that up to now.

We would call that new method form openFile: openUrl: and openTempFile:.

I am still unsure about all that. Thinking this stuff over Richards solution to provide a GNUstep wrapper for xdg-open seems a lot more attractive. We will need something similar for Windows wrapping that ShellExectureW() call, here writing a short tool should do.

Sorry, lets better postbone this until after the release that gives me time to play a bit with the wrapper idea.

Fred Kiefer <FredKiefer>
Group Member
Sun 22 Jan 2012 07:49:05 PM UTC, comment #18: 

could this patch please make it into the upcoming release? i'd rather not have to depend on a patched gnustep version just because of this one issue...

julian <julian_>
Mon 21 Nov 2011 10:09:24 AM UTC, comment #17: 

ok i have reworked the patch.

  • still uses NSTask on unix and native API ShellExecute on win32.
  • actually works now
  • as suggested is user configurable, the user defaults keys GSOpenFileTools and GSOpenURLTools should contain arrays of strings


some things are missing still

  • the patch assumes the talked-about changes to NSPerformServices(). i'd consider it a bug that an API call like openURL would perviously throw up an alert in the users-face,  but now the situation is even worse because you can get the error dialog and still have it succeed because the native-path is now the last fallback even in the openURL case.
  • the patch makes sense only if GSOpenFileTools and GSOpenURLTools are set to sensible defaults, but i don't know how to add default values for the user defaults. i suggest setting both to "("xdg-open", "gnome-open", "kde-open")"



(file #24412)

julian <julian_>
Sat 12 Nov 2011 06:55:31 PM UTC, comment #16: 

sorry  the NSTask version is bugged, additionally i'd like more graceful recovery if xdg-open doesn't exist, including fallback to gnome-open and kde-open. will work on this next week.

julian <julian_>
Sat 05 Nov 2011 08:17:55 PM UTC, comment #15: 

well i updated it to use NSTask, what else is needed to get it in? ;)

julian <julian_>
Sat 22 Oct 2011 09:52:32 PM UTC, comment #14: 

should have clarified, new patch still uses ShellExectureW() on WIN32 and NSTask/xdg-open elsewhere

julian <julian_>
Sat 22 Oct 2011 09:50:51 PM UTC, comment #13: 


>regarding system(), I think the main problem is format string vulnerability


well i am not sure this is a real problem, also validating the string shouldn't be hard

but i've updated the patch to use NSTask

btw. if the code you talked about is in GSGhostscriptImageRep, it seems the code to locate the executables is redundant with the GNUstep NSTask extension [NSTask+launchPathForTool:]

(file #24187)

julian <julian_>
Sat 22 Oct 2011 09:02:57 PM UTC, comment #12: 

ShellExecuteW() sounds like the right choice on win32 to me.

regarding system(), I think the main problem is format string vulnerability… i.e. a malicious user sends you a file with a specially-consturcted filename and tells you to open it, and the shell ends up executing a code snippet in the filename.

for fun, here is an example with an evil filename which deletes a file called test.xxx in the current directory (you can actually create a file with that name, at least on os x):

#include <stdlib.h>
#include <stdio.h>

void innocent_open_file(char *file)
{
char buf[1024];
sprintf(buf, "echo '%s'", file);

printf("calling system(\"%s\");\n", buf);
system(buf);
}

int main(int argc, const char **argv)
{
char *evilfilename = "deleting file...'; rm test.xxx; echo 'done.";
innocent_open_file(evilfilename);
return 0;
}

Eric Wasylishen <ericw>
Group Member
Sat 22 Oct 2011 07:59:51 PM UTC, comment #11: 

hmmm. i think the ShellExectureW() call is the right way to open a document/URL on windows, so it should work better than NSTask'ing "Explorer.exe /e" ... so porting the stuff to use NSTask would actually make it worse. (aren't there stripped systems without explorer exe too?)
but its your call. if you say you want to make it worse i will update the patch.

>I am a bit unsure about the part of the patch that changes the behaviour for URLs.


i actually don't want to change the behavior there. i would like to add the native integration as the last fallback path for openURL: too, but the thing is that NSPerformService() throws a dialogue in the users face if it doesn't succeed. if that behavior is changed, NSPerformService can be tried before the fallback native system.

julian <julian_>
Sat 22 Oct 2011 07:42:53 PM UTC, comment #10: 

I wrote some code in gui for launching ghostscript using NSTask which could be borrowed for this (it handles searching through the PATH variable to find the requested executable, and handles the different PATH separators on win32 vs posix. Perhaps that could be moved into a private method on NSTask or NSWorkspace, since it useful in general?)

Eric Wasylishen <ericw>
Group Member
Sat 22 Oct 2011 12:22:11 PM UTC, comment #9: 

I am willing to add a patch similar to this one. But like Richard I would prefer a solution that uses NSTask to start and external process. On Windows we could use the command "Explorer.exe /e, " followed by the file name to start the corresponding executable. No idea why there is a comma in the command, but this seems to work.
The solution I would implement would check for a general command defined in the user defaults and use the system specific fall backs.

I am a bit unsure about the part of the patch that changes the behaviour for URLs. There the URL service wont be used, even if one is defined. Is this the right thing to do?

Fred Kiefer <FredKiefer>
Group Member
Thu 20 Oct 2011 03:17:46 PM UTC, comment #8: 

since this is the only patch i depend on, i'd really like to see (something like) this upstream. what would still be necessary to get this comitted?

julian <julian_>
Thu 06 Oct 2011 10:58:32 AM UTC, comment #7: 

attached a version that should apply cleanly, win32 codepath still untested.

(file #24111)

julian <julian_>
Thu 06 Oct 2011 12:07:10 AM UTC, comment #6: 

now without memleak

(file #24110)

julian <julian_>
Thu 06 Oct 2011 12:05:15 AM UTC, comment #5: 

the new patch may not apply cleanly since i don't have access to the linux machine ATM i just added the new lines to the old patch. i think you will be able to sort it out.

julian <julian_>
Thu 06 Oct 2011 12:03:19 AM UTC, comment #4: 

new patch with experimental native code path for win32 which uses ShellExecuteW().

can't verify the win32 path, may also need an include of <Shellapi.h>

(file #24109)

julian <julian_>
Wed 05 Oct 2011 11:30:20 PM UTC, comment #3: 


>GWorkspace


i don't think these APIs should depend on GWorkspace (i don't have it) and the idea of having to wrap any linux app in existence ...  sounds not so bright.


>a. not work on any system without xdg-open


AFAIK all linux distributions in the last few years have shipped it, and even if it isn't there .. is just a fallback

>b. will prevent the code building on mswindows (no system function)


an ifdef will fix that - there is no xdg-open on windows anyway.

> NSTask


i don't see a value in using a more complicated and problematic API in order to ensure portability to a platform where xdg-open doesn't run anyway. i'm sure windows has some native APIs to open file and URLs and this can be added as an #else later on

julian <julian_>
Wed 05 Oct 2011 11:06:06 PM UTC, comment #2: 

Clearly part of the problem here is lack of (and/or hard to find) documentation of how to set up file-type=>app mappings and a lack of default mappings, but it would be nice to have better integration with any existing mechanism on whatever platform the software happens to be running.

Richard Frith-Macdonald <CaS>
Group Member
Wed 05 Oct 2011 10:58:29 PM UTC, comment #1: 

First ... 'if they don't find a GNUSTEP application to open the file / url, they just fail - even if non-GNUSTEP apps to open the file/url are installed' is not accurate ... in fact they can open any app whether gnustep or not ... you just have to set up a wrapper (an app bundle without an actual app ... just the info plist to say what to execute) which will run the non-gnustep app.  GWorkspace includes wrappers for lots of common apps, but you could also have a wrapper to call xdg-open for some (many?) file extensions.
Perhaps there should be demo wrappers included with the gui library rather than depending on GWorkspace to provide them?

Second ... the proposed change will of course
a. not work on any system without xdg-open
b. will prevent the code building on mswindows (no system function)

But the idea for a fallback to handle unknown file types is good:
A portable option would be to use NSTask to launch a task, and have a user default to specify the command to be launched, so it could easily be customised to something other than xdg-open.
That might not be enough for windows (I don't know what the preferred mechanism is there), so maybe conditional compilation of using an mswindows specific api to lookup an app to launch would be needed.

Richard Frith-Macdonald <CaS>
Group Member
Wed 05 Oct 2011 09:59:03 PM UTC, original submission:  

the NSWorkspace method families

- (BOOL)openFile:(NSString *)fullPath
- (BOOL)openURL:(NSURL *)url

are implemented in a quite useless way. if they don't find a GNUSTEP application to open the file / url, they just fail - even if non-GNUSTEP apps to open the file/url are installed. this may be useful in a gnustep desktop environment, but is completely counter-productive when deploying a gnustep-using app in a gnome/kde/xfce/whatever world.

attached patch fixes the issue by opening the file/url with xdg-open, the freedesktop.org standard for opening files/urls. only as a fallback of course.


julian <julian_>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #24412:  workspaceopen.patch added by julian_ (3KiB - application/octet-stream)
file #24187:  open_exp4.txt added by julian_ (2KiB - text/plain)
file #24111:  open_exp3.diff added by julian_ (2KiB - application/octet-stream)
file #24110:  open_exp2.diff added by julian_ (2KiB - application/octet-stream)
file #24109:  open_exp.diff added by julian_ (2KiB - application/octet-stream)
file #24107:  open.diff added by julian_ (1KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rmottola (Updated the item)
  • -email is unavailable- added by gcasa (Posted a comment)
  • -email is unavailable- added by ericw (Posted a comment)
  • -email is unavailable- added by FredKiefer (Posted a comment)
  • -email is unavailable- added by julian_ (Voted in favor of this item)
  • -email is unavailable- added by CaS (Posted a comment)
  • -email is unavailable- added by julian_ (Submitted the item)
  •  

    There are 5 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-01-15 rmottola Open/ClosedIn Test Closed
    2012-06-29 FredKiefer StatusNone Ready For Test
        Assigned toNone FredKiefer
        Open/ClosedOpen In Test
    2011-11-21 julian_ Attached File- Added workspaceopen.patch, #24412
    2011-10-22 julian_ Attached File- Added open_exp4.txt, #24187
    2011-10-06 julian_ Carbon-Copy- Added julian_
    2011-10-06 julian_ Attached File- Added open_exp3.diff, #24111
    2011-10-06 julian_ Attached File- Added open_exp2.diff, #24110
    2011-10-06 julian_ Attached File- Added open_exp.diff, #24109
    2011-10-05 CaS Item GroupNone Change Request
        Summary-[NSWorkspace open*] implemented uselessly -[NSWorkspace open*] native system integration
    2011-10-05 julian_ Attached File- Added open.diff, #24107

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code