Fix incorrect behavior of 'error' function #303
Conversation
- Add proper bounds checking to prevent buffer overflows - Fix line/column position calculation logic - Add line number reporting for better debugging - Handle edge cases (empty source, NULL pointers) - Improve error message formatting and accuracy Fixes sysprog21#236
|
Read https://github.com/sysprog21/shecc/blob/master/CONTRIBUTING.md carefully. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="src/globals.c">
<violation number="1" location="src/globals.c:1409">
Bounds check unconditionally shifts the error position one character left, causing off-by-one error in caret/column reporting.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| char diagnostic[512]; /* MAX_LINE_LEN * 2 */ | ||
|
|
||
| /* Ensure current_pos is within bounds */ | ||
| if (current_pos >= SOURCE->size) { |
There was a problem hiding this comment.
Bounds check unconditionally shifts the error position one character left, causing off-by-one error in caret/column reporting.
Prompt for AI agents
Address the following comment on src/globals.c at line 1409:
<comment>Bounds check unconditionally shifts the error position one character left, causing off-by-one error in caret/column reporting.</comment>
<file context>
@@ -1381,36 +1381,91 @@ void fatal(char *msg)
+ char diagnostic[512]; /* MAX_LINE_LEN * 2 */
+
+ /* Ensure current_pos is within bounds */
+ if (current_pos >= SOURCE->size) {
+ current_pos = SOURCE->size - 1;
+ }
</file context>
| if (current_pos >= SOURCE->size) { | |
| if (current_pos > SOURCE->size) { |
| /* Output the error with improved formatting */ | ||
| printf("[Error]: %s\n", msg); | ||
| printf("At line %d, column %d (source position %d):\n", | ||
| line_number, error_column + 1, current_pos); | ||
| printf("%s\n", diagnostic); |
There was a problem hiding this comment.
This does not resolve the goal of TODO, as the line number and column number could only be meaningful when file name persists. With only line number and column number it's quite meaningless.
| if (!SOURCE || !SOURCE->elements || SOURCE->size < 0) { | ||
| printf("[Error]: %s\nSource location unavailable (invalid source buffer)\n", msg); | ||
| abort(); | ||
| } |
There was a problem hiding this comment.
This does not correctly handle OOB issue.
| char diagnostic[512 /* MAX_LINE_LEN * 2 */]; | ||
| /* Safety check for NULL message */ | ||
| if (!msg) { | ||
| printf("[Error]: Unknown error occurred\n"); |
There was a problem hiding this comment.
What's the purpose of adding a NULL check here?
AFAICS there is no case where a NULL pointer is passed in, so this doesn't address any real security issue.
Also, I don't see a strong reason to change the API to allow NULL pointers as valid input.
| int current_pos = SOURCE->size; | ||
| int line_start, line_end; | ||
| int i = 0; | ||
| char diagnostic[512]; /* MAX_LINE_LEN * 2 */ |
There was a problem hiding this comment.
Personally, I would prefer keeping all variable declarations at the top of the function.
Problem
The
errorfunction inglobals.chad several critical issues:Solution
Testing
Example Output
Before:
[Error]: Unexpected token
Occurs at source location 42.
After:
[Error]: Unexpected token
At line 3, column 15 (source position 42):
int x = invalid syntax;
^ Error occurs here
Fixes #236
Summary by cubic
Fixes error() to report the correct line and column with a clear caret diagnostic, addressing #236. Adds bounds checks and safe handling of empty source and NULL pointers to prevent crashes and buffer overflows.