Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8181571: printing to CUPS fails on mac sandbox app #4861

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

@AlexanderScherbatiy
Copy link

@AlexanderScherbatiy AlexanderScherbatiy commented Jul 21, 2021

The issue is reproduced on macOS Big Sur 11.0.1 with jdk 16.0.1+9.

Create a native macOS app from the Hello.java file, sign and run it in sandbox:

import javax.print.*;
import javax.swing.*;

public class Hello {

    public static void main(String[] args) throws Exception {
        SwingUtilities.invokeAndWait(() -> {
            boolean isSandboxed = System.getenv("APP_SANDBOX_CONTAINER_ID") != null;
            PrintService defaultPrinter = PrintServiceLookup.lookupDefaultPrintService();
            PrintService[] services = PrintServiceLookup.lookupPrintServices(null, null);

            StringBuilder builder = new StringBuilder();
            builder.append("is sandboxed: ").append(isSandboxed).append("\n");
            builder.append("default printer: ").append(defaultPrinter).append("\n");
            int size = services.length;
            for (int i = 0; i < size; i++) {
                builder.append("printer[").append(i).append("]=").append(services[i]).append("\n");
            }
            JOptionPane.showMessageDialog(null, builder.toString());
        });
    }
}

The signed app in sandbox shows null default printer and PrintServiceLookup.lookupPrintServices(null, null) returns "Unix Printer: lp".
PrintSandboxedApp

The problem has been discussed on 2d-dev mail list:
https://mail.openjdk.java.net/pipermail/2d-dev/2017-June/008375.html
https://mail.openjdk.java.net/pipermail/2d-dev/2017-July/008418.html

According to the discussion:

I've submitted a DTS incident to Apple and a friend there has followed-up.
Their unofficial position is that java should be connecting to the cups interface returned
by the cupsServer() function and not changing the interface string to "localhost".
Security changes in 10.12.4 reject the TCP connection which they say confuses
network-client access with print access. They don't seem interested in loosening that change.

The proposed solution is to use the domain socket pathname in httpConnect(...) cups function and cupsGetDests(...) to get list of printers from cups when the app is signed and is run in sandbox on MacOs.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8181571: printing to CUPS fails on mac sandbox app

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4861/head:pull/4861
$ git checkout pull/4861

Update a local copy of the PR:
$ git checkout pull/4861
$ git pull https://git.openjdk.java.net/jdk pull/4861/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4861

View PR using the GUI difftool:
$ git pr show -t 4861

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4861.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 21, 2021

👋 Welcome back alexsch! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Jul 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 21, 2021

@AlexanderScherbatiy The following label will be automatically applied to this pull request:

  • 2d

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the 2d label Jul 21, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 21, 2021

Webrevs

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Jul 21, 2021

It is not clear how to write a test (automated or manual) for the fix.
To check the fix it is necessary to sign a native app on macOS, install it, and run it.

I run automated and manual test/jdk/java/awt/print and java/awt/PrintJob tests on macOS Big Sur 11.0.1 with the fix but they do not test the new behavior where a domain socket path name is used because tests are not run in sandbox.

The automated test/jdk/java/awt/print and java/awt/PrintJob tests pass.

The following manual tests fail with and without the fix:

java/awt/print/Dialog/DialogCopies.java                                               Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: Copies not incremented
java/awt/print/Dialog/PrintDlgApp.java                                                Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: No file created
java/awt/print/PrinterJob/PageDlgApp.java                                             Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: PrinterJob.pageDialog(PrintRequestAttributeSet) does not return null when dialog is cancelled
java/awt/print/PrinterJob/PrintDialogCancel.java                                      Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: User pressed cancel, but true returned
java/awt/print/PrinterJob/RadialGradientPrintingTest.java                             Failed. Unexpected exit from test [exit code: 0]
java/awt/print/Dialog/DialogOrient.java                                               Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/print/Dialog/DialogType.java                                                 Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/print/Dialog/PrintDlgPageable.java                                           Error. java.lang.IllegalAccessException: class com.sun.javatest.regtest.agent.MainWrapper$MainThread cannot access a member of class PrintDlgPageable with modifiers "public static"
java/awt/print/PrinterJob/ImagePrinting/ClippedImages.java                            Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/print/PrinterJob/ImagePrinting/ImageTypes.java                               Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/print/PrinterJob/ImagePrinting/PrintARGBImage.java                           Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/print/PrinterJob/PageDialogTest.java                                         Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/print/PrinterJob/PageRanges.java                                             Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/print/PrinterJob/PageRangesDlgTest.java                                      Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/print/PrinterJob/PrintGlyphVectorTest.java                                   Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/print/PrinterJob/PrintLatinCJKTest.java                                      Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/print/PrinterJob/PrintTextTest.java                                          Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/print/PrinterJob/SwingUIText.java                                            Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/PrintJob/ConstrainedPrintingTest/ConstrainedPrintingTest.java    Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/PrintJob/PageSetupDlgBlockingTest/PageSetupDlgBlockingTest.java  Error. Parse Exception: Arguments to `manual' option not supported: yesno
java/awt/PrintJob/PrintCheckboxTest/PrintCheckboxManualTest.java          Error. Can't find HTML file: /Users/sample/openjdk/jdk-cups/test/jdk/java/awt/PrintJob/PrintCheckboxTest/PrintCheckboxManualTest.html
java/awt/PrintJob/SaveDialogTitleTest.java                                Error. Parse Exception: Arguments to `manual' option not supported: yesno

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Jul 22, 2021

/label awt,2d

@openjdk openjdk bot added the awt label Jul 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 22, 2021

@AlexanderScherbatiy
The awt label was successfully added.
The 2d label was already applied.

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Jul 22, 2021

/cc 2d

@openjdk
Copy link

@openjdk openjdk bot commented Jul 22, 2021

@AlexanderScherbatiy The 2d label was already applied.

@NikolayTach
Copy link

@NikolayTach NikolayTach commented Jul 28, 2021

JDK-8247768
Same here played 6 times yet moved, 8 issues missed.

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Aug 5, 2021

JDK-8247768
Same here played 6 times yet moved, 8 issues missed.

What is the relation between JDK-8247768 and the current pull request?
Should something be updated in the pull request description?

private static String getDomainSocketPathname() {
return domainSocketPathname;
}

Copy link
Contributor

@prrace prrace Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to suppress deprecation warnings here.

Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add @SuppressWarnings("deprecation") to the getDomainSocketPathname() method?

IPPPrintService.debug_println(debugPrefix+"CUPS server "+getServer()+
" port "+getPort());
return canConnect(getServer(), getPort());
String server = getDomainSocketPathname() != null ? getDomainSocketPathname() : getServer();
Copy link
Contributor

@prrace prrace Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split this long line

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@@ -92,6 +94,13 @@ public Void run() {
libFound = initIDs();
if (libFound) {
cupsServer = getCupsServer();
Copy link
Contributor

@prrace prrace Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should wrap all the new lines in isMac()
Also can you explain the reasons for the logic that implies we may have a server starting with "/"
in which case your always change the cupServer to localhost even if it is not sandboxed ?

I ask because it seems to contradict what you pasted
"by the cupsServer() function and not changing the interface string to "localhost""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic which replaces a server starting with "/" to localhost is the original logic which is implemented in native level in the getCupsServer() function:

if (strncmp(server, "/", 1) == 0) {

The fix only moves this logic to the java level to store domainSocketPathname in case of sandboxed app on MacOS.

}
return printerURIs;
}
}
Copy link
Contributor

@prrace prrace Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if getCupsDefaultPrinters() doesn't find anything you always continue to the original code even though
it would seem that you know we are in a sandboxed app and it won't find anything ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the code to return null in case there are no printer names from j2d_cupsGetDests() function in a sandboxed on MacOS.

j2d_cupsFreeDests(num_dests, dests);
DPRINTF("CUPSfuncs::bad alloc new array\n", "")
(*env)->ExceptionClear(env);
JNU_ThrowOutOfMemoryError(env, "OutOfMemoryError");
Copy link
Contributor

@prrace prrace Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this weird. What is the ExceptionClear for ? The only possible exception here is for
an OOME which might be thrown by NewObjectArray. So you clear that and then re-create it ?
And who do will catch this ? What's the point ? Maybe just clear and return null.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array creation error handling is implemented in the similar way as it is done in the getMedia() function.
The ExceptionClear() has been added to the getMedia() by 8031001: [Parfait] warnings from b121 for jdk/src/solaris/native/sun/awt: JNI-related warnings
I would prefer to have unified error handling in these methods. If ExceptionClear is not suitable in this place it would be better to recheck JDK-8031001 and update all places accordingly in a separate fix.

if (utf_str == NULL) {
j2d_cupsFreeDests(num_dests, dests);
DPRINTF("CUPSfuncs::bad alloc new string ->name\n", "")
JNU_ThrowOutOfMemoryError(env, "OutOfMemoryError");
Copy link
Contributor

@prrace prrace Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comments to above plus I am fairly sure you want to delete nameArray since it isn't returned.
For that matter if the NewString fails on the 4th printer you want to free the 3 previous ones too before returning.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is updated to remove previously created strings and clear the nameArray in case of an error during new string creation.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 22, 2021

@AlexanderScherbatiy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Sep 29, 2021

/label client

@openjdk
Copy link

@openjdk openjdk bot commented Sep 29, 2021

@AlexanderScherbatiy
The client label was successfully added.

@openjdk openjdk bot added the client label Sep 29, 2021
@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Sep 29, 2021

/label remove awt,2d

@openjdk
Copy link

@openjdk openjdk bot commented Sep 29, 2021

@AlexanderScherbatiy The label awt is not a valid label. These labels are valid:

  • serviceability
  • hotspot
  • hotspot-compiler
  • ide-support
  • kulla
  • i18n
  • shenandoah
  • jdk
  • javadoc
  • security
  • hotspot-runtime
  • jmx
  • build
  • nio
  • client
  • core-libs
  • compiler
  • net
  • hotspot-gc
  • hotspot-jfr

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Oct 8, 2021

Just some more details about code in CUPSPrinter class static initialization.

The getCupsServer() native method from CUPSPrinter calls j2d_cupsServer() function to get the cups server. If the server name starts with "/" it is replaced to "localhost"

if (strncmp(server, "/", 1) == 0) {

To keep the domain socket path name from j2d_cupsServer() call the fix moves the cups server name handling to the java side. The original domain socket path name is preserved in the CUPSPrinter class only for MacOS when sandboxed app is used.

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 5, 2021

@AlexanderScherbatiy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Nov 24, 2021

Just a question. Should the fix have 2 reviewers?

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 22, 2021

@AlexanderScherbatiy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Jan 14, 2022

I see that the fix has been inactive for several months.
Is there something that I missed and should update from my side (add comments, answer on questions, update commits, or something else)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants