Skip to content

fix-tmp-symlink #3359

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix-tmp-symlink #3359

wants to merge 3 commits into from

Conversation

risdenk
Copy link
Contributor

@risdenk risdenk commented May 22, 2025

https://issues.apache.org/jira/browse/SOLR-XXXXX

Description

On Mac /tmp is a symlink and this causes issues with the Solr security manager. SOLR-16457 / #1282 fixed a similar issue with symlinks and the security manager. This applies the same type of solution but specifically for java.io.tmpdir to ensure that on Macs (and Linux) the full path is used not the symlink. This makes the security manager checking the path happy since its not trying to compare to a symlink anymore.

This assumes that TMPDIR is already defined which looks like it is probably the case, but not 100% sure. Otherwise we fall back to TEMP and then TMP and finally /tmp. If someone wants to override the tmp dir they should set TMPDIR and not try to just set -Djava.io.tmpdir...

References:

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

@risdenk risdenk self-assigned this May 22, 2025
Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

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

So, this makes total sense. My only thought is that it's adding a bit more complexity to this already incredibly complex script! If we removed Security Manager, would that obviate the need for this? And, if that statement is true, how can we make sure to back this change out in the future when that happens?

Copy link
Contributor

@malliaridis malliaridis left a comment

Choose a reason for hiding this comment

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

Tested on a Mac and this fixes the issue. It seems that you also covered all places in the solr script, good job! 👍

Have you checked if the cross-dc-manager may be also affected by this bug? I never used the tool before to know how the script works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants