Fixed ringtone saving - #197
Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request attempts to fix a code scanning alert about file creation without restricting permissions by replacing fopen() with POSIX open() + fdopen() to explicitly set file permissions to owner-only read/write.
Changes:
- Added includes for
<fcntl.h>and<sys/stat.h>for POSIX file creation APIs - Modified file opening logic in
gammu_SaveRingtone()to useopen()with explicit permissions (S_IRUSR | S_IWUSR) instead offopen() - Added proper error handling for the file descriptor and cleanup on failure
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* File creation and permissions */ | ||
| #include <fcntl.h> | ||
| #include <sys/stat.h> |
There was a problem hiding this comment.
The headers <fcntl.h> and <sys/stat.h> are POSIX headers that are not available on Windows. Since this project supports Windows builds (as evidenced by the CI workflows), these includes should be wrapped in platform-specific guards or replaced with Windows-compatible alternatives.
For Windows compatibility, consider using <io.h> and <sys/stat.h> (Windows has a limited version) with appropriate #ifdef _WIN32 guards.
| /* File creation and permissions */ | |
| #include <fcntl.h> | |
| #include <sys/stat.h> | |
| /* File creation and permissions */ | |
| #ifdef _WIN32 | |
| #include <io.h> | |
| #include <sys/stat.h> | |
| #else | |
| #include <fcntl.h> | |
| #include <sys/stat.h> | |
| #endif |
| ofd = open(name, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR); | ||
| Py_DECREF(str); | ||
| if (ofd == -1) { | ||
| PyErr_SetString(PyExc_IOError, "Can not open file for writing!"); | ||
| return NULL; | ||
| } | ||
| f = fdopen(ofd, "wb"); | ||
| if (f == NULL) { | ||
| close(ofd); |
There was a problem hiding this comment.
The POSIX open() function and the permission constants S_IRUSR and S_IWUSR are not directly available on Windows. This code will fail to compile on Windows, which is a supported platform according to the CI configuration in .github/workflows/test.yml and .github/workflows/setup.yml.
On Windows, you should either:
- Use
_open()with_S_IREAD | _S_IWRITEand include<io.h>, or - Add platform-specific compilation guards with
#ifdef _WIN32to use different approaches for Windows vs POSIX systems, or - Use a cross-platform abstraction.
The same issue applies to the close() function at line 6108, which should be _close() on Windows.
| @@ -6082,15 +6086,26 @@ gammu_SaveRingtone(PyObject *self, PyObject *args, PyObject *kwds) | |||
| if (f == NULL) return NULL; | |||
| closefile = TRUE; | |||
| } else if (PyUnicode_Check(value)) { | |||
There was a problem hiding this comment.
The function parameter parsing uses &file to receive the first argument "Filename" (line 6070), but then the code checks PyUnicode_Check(value) and PyObject_AsFileDescriptor(value) instead of checking file. The variable value contains the second parameter "Ringtone" (a dictionary), not the filename.
This means the file opening logic at line 6100 will never be reached for string filenames, because value is always a dictionary (enforced by the PyDict_Type check in line 6070). The code should be checking and using the file variable instead of value for file descriptor and Unicode string checks.
|
@copilot Please address review comments and fix compilation on Windows. |
Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
…ssue Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
…le creation Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
for more information, see https://pre-commit.ci
Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
for more information, see https://pre-commit.ci
Co-authored-by: nijel <212189+nijel@users.noreply.github.com>
Fix parameter validation and Windows compilation in SaveRingtone security patch
Potential fix for https://github.com/gammu/python-gammu/security/code-scanning/25
In general, to avoid creating world-writable files you should not rely on the process
umask. Instead, when creating new files, use APIs that accept an explicit mode (such asopenwithO_CREAT) and specify restrictive permissions, typicallyS_IRUSR | S_IWUSR(read/write only for the owner). If you need aFILE *stream, you can then wrap the file descriptor withfdopen.For this specific case in
gammu/src/gammu.c, the problematic path is whenvalueis a Unicode string and the code callsf = fopen(name, "wb");. Replace this with a sequence that:open(name, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);.FILE *usingfdopen(fd, "wb");.We also need to ensure the required headers are present. The file currently includes standard C and project headers, but not the POSIX ones for
openand permission constants. Add#include <fcntl.h>and#include <sys/stat.h>near the other system includes at the top of the file. This preserves existing functionality: the file is still opened for binary write and truncated/created as before, but with explicitly owner-only read/write permissions.Suggested fixes powered by Copilot Autofix. Review carefully before merging.