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

Fix buffer overrun on long TMPDIR path #1017

Merged
merged 1 commit into from Mar 7, 2019

Conversation

@riyazhakki
Copy link
Member

commented Mar 6, 2019

Bug/feature Description

  • If the user define the TMPDIR path longer than ~128 characters then job submission fails with error 'qsub: cannot access script file'
  • Steps to reproduce:
  1. export TMPDIR=/home/mohammh/aaaaaaaaaa/bbbbbbbbbb/cccccccccc/eeeeeeeeee/ffffffffff/gggggggggg/hhhhhhhhhh/iiiiiiiiii/jj/afdj/hlppoo/jkloiytupoo/bhtiusabsdlg/
  2. mkdir -p $TMPDIR
  3. qsub job.scr
    qsub: cannot access script file

Affected Platform(s)

  • All Linux

Cause / Analysis / Design

  • Function get_comm_filename appends the tmpdir path to the path of unixsocket filename, which is restricted to 108 charecters and thus when the user update the TMPDIR path longer than 108 characters, buffer overflow occurs and that leads to invalid path.

Solution Description

  • Using snprintf to avoid the bufferflow and if the tmpdir path is greater than buffer size we use /var/tmp path for the unixsocket file.

Testing logs/output

test_qsub_with_script_with_long_TMPDIR_BeforeFix.ptl.txt
test_qsub_with_script_with_long_TMPDIR_AfterFix.ptl.txt
TestQsubOptionsArguments.ptl.txt

Checklist:

For further information please visit the Developer Guide Home.

@riyazhakki riyazhakki force-pushed the riyazhakki:buffer_overflow branch from 75e77ea to f73885a Mar 6, 2019

@riyazhakki riyazhakki marked this pull request as ready for review Mar 6, 2019

@prakashcv13
Copy link
Contributor

left a comment

@riyazhakki - tmpdir is being used in get_script() as well.
Also, the solution that you are proposing would silently override the TMPDIR setting in qsub.c, which the user might not know or expect, which I do not think is what we would want.
How about using dynamically allocated memory for fname (get_comm_filename) and tmp_name (get_script). If memory allocation fails, we display "out of memory" and exit.

@mike0042

This comment has been minimized.

Copy link
Collaborator

commented Mar 6, 2019

Dynamically allocated memory won't make a difference in this case. The limitation is on the length of the named pipe which is restricted to 108 characters in /usr/include/linux/un.h. The full path must fit in the sockaddr_un.sun_path array. We would only use the alternate location in the unlikely event that the user has defined a very long TMPDIR. The only other solution I can think of would be to create a (much shorter) symbolic link to the named pipe and use that instead, but I prefer the solution being proposed.

@riyazhakki

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

Thank you Mike for providing clarification. Prakash, about tmpdir in get_script () we are not changing the tmpdir for the job script.

@prakashcv13

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@mike0042 - I did not realize we are using this path for the Unix domain socket. Thank you for pointing out.
@riyazhakki - in get_script() we do use tmpdir which may have the value from the environment variable. As far as I know, env variables can be more than 1024 (MAXPATHLEN) characters. If the user sets the TMPDIR to a path more than 1024 characters which is less than PATH_MAX (4096), mkstemp() will fail.

here is an example run.
qsub buffer overrun.txt

This may not cause a buffer overrun, as we are already using snprintf().
We can use the same logic as you have in get_comm_filename().

@riyazhakki

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

Prakash, this environment variable we are using for the file path and I think most of the filesystems limit it to the 255 bytes, even in case user provide the long path we are doing right throwing the message "qsub: could not create/open tmp file".

@prakashcv13

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

filenames are limited to 255 characters. Most of the modern FSs do not have a limit on the path length. This is a comparison of the FSs.
Coming to doing the right thing - the user is in no way aware that qsub will be copying the script to a temporary location, so this error will not be something expected. Copying the file is internal to qsub.

@riyazhakki

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

I think it is long odds the user will provide the path greater than MAXPATHLEN as compare to MAXPIPNAME (for unix pipe).
I would like to go with existing code until the reviewer strongly feels we need the similar change here.

@subhasisb

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2019

@prakashcv13 for the temporary script file, we do not want to override if user has set anything in TMPDIR. That is because the user wants us to use TMPDIR for all temporary stuff..script files might e large etc. However, The the domain socket filename is different, it's internal and 0 bytes in length, so we chose to try /tmo before we give up. I don't think we should do that for scripts - it current thows an error which can be enhanced, but that is probably a separate ticket

@prakashcv13

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@subhasisb - copying the script too is internal to qsub, which makes the error message unexpected for the user.
However, I do see a point in not overriding TMPDIR for copying scripts because it beats the reasons for which the TMPDIR was set to something else in the first place. How we go about handling it, can be done as a separate ticket as you suggested.

@subhasisb

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2019

@subhasisb - copying the script too is internal to qsub, which makes the error message unexpected for the user.
However, I do see a point in not overriding TMPDIR for copying scripts because it beats the reasons for which the TMPDIR was set to something else in the first place. How we go about handling it, can be done as a separate ticket as you suggested.

@prakashcv13 , You are right - both of these things are actually internal. However, the socket filename is zero byte file whereas the temporary script might take actual space. The socket filename length is limited by our choice of the communication infrastructure - which is the unix domain socket. On the other hand, the temporary file path is just that a path with a maximum allowed length. In that sense, the socket filename path is more "internal" implementation dependant that the temporary script path...and thus this attempt to work around the issue.

@prakashcv13

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@subhasisb - alright, let us go ahead with this workaround.

@subhasisb subhasisb merged commit 3ba74c4 into PBSPro:master Mar 7, 2019

4 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@riyazhakki

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

Thank you Subhasis and Prakash for the review and approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.