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

use globbing instead of find #409

Closed
wants to merge 1 commit into from

Conversation

dennisse
Copy link

linuxserver.io


  • I have read the contributing guideline and understand that I have made the correct modifications

Description:

Use bash globbing instead of find.

find doesn't seem to recognize any of the files in my /dev/dri-folder as character special files. Bash's [[ -c file ]] does.

root@061899068dcd:/dev/dri# find /dev/dri -type c
root@061899068dcd:/dev/dri# for f in /dev/dri/*; do [[ -c "$f" ]] && echo "$f is a special character file"; done
/dev/dri/card0 is a special character file
/dev/dri/renderD128 is a special character file

Also, using globbing should be a little bit safer. It's hard to loop over files from another program. So hard it's one of the top bash pitfalls. Using globbing is simpler and cleaner.

Benefits of this PR and context:

A little bit cleaner code, that also works on my system.

How Has This Been Tested?

I ran the script.

It's not a big change :)

podman version 5.2.3, on Debian Testing. Image id 88b59fbfee1f

Source / References:

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thanks for opening this pull request! Be sure to follow the pull request template!

@aptalca
Copy link
Member

aptalca commented Oct 10, 2024

It's not a big change :)

It is when you consider how many images we publish use the same code. You should look into why in your environment find doesn't find them as we can't reproduce it.

@LinuxServer-CI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/plex/1.41.0.8994-f2c27da23-pkg-31d1465e-dev-24897def28c458d7b4e8a87b949c6b31f603e1f0-pr-409/index.html
https://ci-tests.linuxserver.io/lspipepr/plex/1.41.0.8994-f2c27da23-pkg-31d1465e-dev-24897def28c458d7b4e8a87b949c6b31f603e1f0-pr-409/shellcheck-result.xml

Tag Passed
amd64-1.41.0.8994-f2c27da23-pkg-31d1465e-dev-24897def28c458d7b4e8a87b949c6b31f603e1f0-pr-409
arm64v8-1.41.0.8994-f2c27da23-pkg-31d1465e-dev-24897def28c458d7b4e8a87b949c6b31f603e1f0-pr-409

@dennisse
Copy link
Author

It is when you consider how many images we publish use the same code.

You're absolutely right.

You should look into why in your environment find doesn't find them as we can't reproduce it.

I absolutely should, and I will.

However, I still think the change is valid. The current implementation with find fails if there is a space or a newline in a device filename, for instance. It also opens up for changing the permissions of any file in the working directory the script is being run from (/run/s6-rc:s6-rc-init:lIndog/servicedirs/s6rc-oneshot-runner (the part after the colon on the dir in /run is random)).

This can be checked by creating a character special device named i.e. card0 notification-fd (i.e. mknod "/dev/dri/card0 notification-fd" c 89 9) and starting the container. If find recognizes the file as a special character device, as it should, /run/s6-rc:s6-rc-init:lIndog/servicedirs/s6rc-oneshot-runner/notification-fd should now be group writeable.

Of course, it isn't very normal with spaces and newlines in device filenames, and it isn't really a security problem, but I don't think that's a good reason not to handle it. Especially when doing so is easier, cleaner, and involves one less fork :)

@aptalca
Copy link
Member

aptalca commented Oct 14, 2024

The link you provided has nothing to do with your current issue. That link is about potential pitfalls when transferring results from one program to another. Your issue is find not returning any results, where nothing is being transferred to begin with.

Our solution has been tested for a long time on various different os baseimages. The only environment we are aware of where our solution does not work is when running docker in lxc (common with proxmox users) but we don't support or recommend that environment. In fact, even the proxmox devs recommend against running docker in lxc as it can cause a lot of other issues.

Replacing our well tested solution with an untested solution to solve one user's (your) issue has too high of a risk to benefit ratio.

@dennisse
Copy link
Author

The link you provided has nothing to do with your current issue. That link is about potential pitfalls when transferring results from one program to another. Your issue is find not returning any results, where nothing is being transferred to begin with.

I didn't mean to imply the link had anything to do with find not finding my character special devices. I was trying to raise the pitfalls of transferring results from one program to another as a second issue. I'm sorry that wasn't clear.

@dennisse dennisse closed this Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants