Replaces ConcNameAndVersion and ConcDirAndName macros with functions #533
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @nbriggs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
I am currently reviewing this pull request. In the meanwhile, I am providing a summary of the content of the pull request for you and other reviewers to get up to speed quickly on the changes that have occurred, and the intent behind these changes.
This pull request replaces the ConcNameAndVersion and ConcDirAndName macros with conc_name_and_version and conc_dir_and_name functions, respectively. The definitions are moved from locfile.h to dsk.c, and they are added to dskdefs.h for use by other source files. This change aims to improve code readability and maintainability by replacing macros with functions.
Highlights
- Macro Replacement: Replaces the
ConcNameAndVersionandConcDirAndNamemacros with functionsconc_name_and_versionandconc_dir_and_name. - Code Relocation: Moves the definitions of these functions from
locfile.htodsk.c. - Header Update: Adds the function declarations to
dskdefs.hto make them accessible to other source files.
Changelog
- inc/dskdefs.h
- Added declarations for
conc_dir_and_nameandconc_name_and_versionfunctions (lines 22 and 23).
- Added declarations for
- inc/locfile.h
- Removed the definitions of the
ConcDirAndNameandConcNameAndVersionmacros (lines 431-467 and 486-495).
- Removed the definitions of the
- src/dsk.c
- Removed include of
locfile.hwhich contained macro definitions (line 30). - Added definitions for
conc_dir_and_name(lines 2544-2580) andconc_name_and_version(lines 2602-2611) functions. - Replaced usages of
ConcNameAndVersionmacro with calls toconc_name_and_versionfunction inCOM_openfile(line 378),DSK_getfilename(lines 890, 930, 979, 1005),DSK_deletefile(line 1145),DSK_renamefile(lines 1308, 1323, 1363),COM_getfileinfo(line 1625),COM_setfileinfo(line 1814),unixpathname(line 832),lisppathname(line 1090), andquote_fname(line 1189) inufs.c. - Replaced usages of
ConcDirAndNamemacro with calls toconc_dir_and_namefunction inDSK_getfilename(lines 969, 998, 1021),DSK_renamefile(line 1297),maintain_version(lines 3309, 3317),get_versionless(line 3422),check_vless_link(line 3501),get_old(lines 3506, 3518, 3546, 3599, 3618, 3705, 3717, 3745, 3770, 3795, 3814, 3826),get_oldest(lines 3792, 3804, 3832, 3857, 3882, 3901, 3913),get_new(lines 3989, 3990, 3999, 4024, 4037, 4053, 4086, 4091, 4134, 4153, 4178, 4191), andget_old_new(lines 4280, 4281, 4288, 4305, 4317, 4333, 4362, 4374, 4413, 4432, 4449, 4461, 4477).
- Removed include of
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Trivia time!
What is the primary difference between a macro and a function in C?
Click here for the answer
Macros are preprocessed, performing textual substitution, while functions are compiled and called at runtime.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request replaces macros with functions, which is a good step towards improving code maintainability and readability. The code is generally well-structured, but there are a few areas that could be improved, particularly regarding string handling safety.
Summary of Findings
- Unsafe string operations: The
conc_name_and_versionfunction usesstrcpyandstrcatwhich are known to be unsafe if the destination buffer is not large enough to hold the concatenated string. This could lead to buffer overflows and potential security vulnerabilities. - DOS #ifdef placement: The placement of the
#ifdef DOSblock insrc/dsk.cwas moved, but it's unclear if this change was intentional or if it affects the code's behavior under DOS.
Assessment
The pull request replaces the ConcNameAndVersion and ConcDirAndName macros with functions, which improves code readability and maintainability. The changes are generally well-structured, but there are a few areas that could be improved. I recommend addressing the comments before merging, and users should have others review and approve this code before merging.
|
/gemini-review |
|
Thanks for the pull request! Here's a summary of the changes and some suggestions. Summary General Comments
I didn't find any specific style guide being followed, so I'll assume you're following general C conventions. Here's a summary of the style guide elements referenced in my review:
|
|
/gemini-review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request replaces macros with functions, which is a good practice for improving code maintainability and readability. The changes also include the use of strlcpy and strlcat to prevent buffer overflows, which is a good security practice.
Summary of Findings
- Buffer size consistency: Ensure that the buffer size passed to
conc_name_and_versionandconc_dir_and_nameis consistent with the actual buffer size. - String concatenation: Consider using
strlcatinstead ofstrcatto prevent buffer overflows.
Assessment
The pull request replaces macros with functions to improve code maintainability and readability. The changes also include the use of strlcpy and strlcat to prevent buffer overflows, which is a good security practice. However, there are some areas where the code can be further improved, such as ensuring that the size parameters passed to conc_name_and_version and conc_dir_and_name are correct and consistent. I recommend addressing these comments before merging, and users should have others review and approve this code before merging.
|
I'm running Maiko off this branch on Linux Mint 22.1 Cinnamon and so far haven't noticed anything unusual. |
|
I actually did notice something unexpected. With Medley on master and Maiko on commit d822d47 of this branch the arrow keys no longer work: Pressing the arrow keys in an Exec or TEdit does nothing and the caret doesn't move. Switching Maiko to master restores the functionality of the arrow keys. I run VNC Medley with |
Replaces ConcNameAndVersion and ConcDirAndName macros with conc_name_and_version and conc_dir_and_name functions respectively, moves the definitions from locfile.h to dsk.c, and adds them to dskdefs.h for use by other source files.
…lace strcpy with strlcpy
…strcpy() with strlcpy()
Moves definitions of the quote_xxx functions from ufs.c where they are not used to dir.c where they are used. Adds a parameter to each of the quote_xxx functions to supply the length of the storage allocated for the result string, and change strcpy() to strlcpy() to avoid potential memory smash if used carelessly.
Unchecked writes to strings using strcpy and strcat can cause memory smashes, replacing them with (destination) bounds checked strl... equivalents can avoid this. Incidentally, fix construction of file name for $HOME/.Xdefaults
Unchecked strcpy/strcat can result in memory smashes if provided with overly long source arguments. Replacing these with the strl... functions protects against overruns of the destination memory. Requires changing the signature of various functions to pass destination storage size when destination argument is a pointer rather than something amenable to sizeof() operator.
…, and increases clarity. * Avoids unnecessary alarm(0) when it is about to be set again, only cleans up after it’s all done. * Renames DIRCHAR to LISPDIRCHAR to be more descriptive, adds LISPDIRSTR for contexts where a string is required rather than a character. * Avoids leaks of directory structures by ensuring that any opendir() has a matching closedir(), with a timeout check, on the return path * Ensures that closedir() calls that could smash errno happen after errno is saved for Lisp’s consumption * Simplifies code to take advantage of strlcpy()/strlcat() guarantee that destination string is NUL terminated * Reduces number of unnecessary copies into scratch buffer for file name processing * Removes some commented out code left over from copy/paste of functions
nbriggs
force-pushed
the
nhb-rewrite-version-parser-v2
branch
from
e50a3ee to
04ad740
Compare
|
I updated to commit 04ad740 and rerun the tests I described in PR #523. Now TEdit works as expected: 
But the file browser breaks with an error when processing the file 
The backtrace: |
|
Yeah, it turns out that the file browser can’t cope with versions over 65535 - it uses a WORD (16 bits) in the data structure to store it. |
* implements these as functions rather than #define macros
* moves definitions to ufs.c
* adds parameter to UnixVersionToLispVersion for the storage
size of the buffer being passed
* removes inc/lispver2.h
* adjusts makefile-dos an makefile-tail to update dependencies
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great step towards improving the robustness and security of the codebase by replacing macros with safer inline functions and updating many string operations to their safer, size-aware counterparts like strlcpy and snprintf. The core changes are well-implemented. I've found one critical issue and a couple of medium-severity issues related to the changes. One is a typo that would likely cause a crash, another is an inconsistency in string handling, and the last is a missing alarm cancellation after a loop. With these fixes, the PR will be in excellent shape.
src/dir.c
Outdated
| len = strlen(namebuf); | ||
| *(nextp->lname + len) = DIRCHAR; | ||
| *(nextp->lname + len) = LISPDIRCHAR; | ||
| *(nextp->lname + len + 1) = '\0'; | ||
| nextp->lname_len = len + 1; |
There was a problem hiding this comment.
This block of code for appending the directory character is inconsistent with the safer string manipulation functions used elsewhere in this PR. It's also potentially buggy: len is calculated from namebuf, but if strlcpy on the previous line truncated the string when copying to nextp->lname, len would be incorrect for nextp->lname. It would be safer and more consistent to use strlcat with LISPDIRSTR here, similar to how it's done in the non-DOS part of this function.
strlcat(nextp->lname, LISPDIRSTR, sizeof(nextp->lname));
nextp->lname_len = strlen(nextp->lname);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great step towards modernizing the codebase by replacing macros with functions and using safer string manipulation functions like strlcpy, strlcat, and snprintf. These changes significantly improve code safety and maintainability. However, I've identified a few critical security vulnerabilities in the newly introduced static functions in dir.c. These functions use fixed-size local buffers without proper size checks, which could lead to buffer overflows. I've provided detailed comments and suggestions for fixing these issues. I also have a suggestion to simplify one of the new functions for better readability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is an excellent refactoring effort that significantly improves code safety and maintainability. The replacement of ConcNameAndVersion and ConcDirAndName macros with functions is a great step. I particularly appreciate the systematic replacement of unsafe string functions like strcpy, strcat, and sprintf with their safer, size-aware counterparts (strlcpy, strlcat, snprintf). Updating function signatures to include buffer sizes further enhances robustness against buffer overflows. The codebase is cleaner and more modern as a result. I have a couple of minor suggestions to improve clarity and prevent future issues.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement. It replaces several macros with safer function-based implementations, notably ConcNameAndVersion and ConcDirAndName. The widespread replacement of unsafe string functions like strcpy, strcat, and sprintf with their safer counterparts (strlcpy, strlcat, snprintf) significantly enhances the code's robustness and security. The changes to function signatures to include buffer sizes are also a welcome addition for preventing buffer overflows. I have one suggestion for improving code readability by refactoring a function to remove goto statements.
nbriggs
dismissed
gemini-code-assist[bot]’s stale review
All necessary changes have been made.
|
I updated to commit 60e12da and now there's an apparent regression as TEdit opens an empty stream when passed the name of the existing ASCII file 
|
|
That's odd -- notice that the TEDIT window title is not the file you asked it to edit. When I do the same thing I get different results. I'd suggest doing a loadup with the newly merged master branch and the latest version of maiko - don't forget to make both the "init" and "x" parts. |
|
I switched to and rebuilt nhb-rewrite-version-parser-v2/60e12da for Maiko and master/82fc95c for Medley and now I get the expected results: 
In my earlier tests I was on one of Ron's Medley branches. |
…est of current_len in conc_dir_and_name
|
I updated to commit 3d09555, still looking good. |
2 participants
Replaces ConcNameAndVersion and ConcDirAndName macros with conc_name_and_version and conc_dir_and_name functions respectively, moves the definitions from locfile.h to dsk.c, and adds them to dskdefs.h for use by other source files.
This is not actually ready to merge - testing Google Copilot analysis of pull requests for maiko code.