fix: Solve multiple sprintf() buffer overflows in dsinterval.c and yminterval.c#1156
fix: Solve multiple sprintf() buffer overflows in dsinterval.c and yminterval.c#1156NotHimmel wants to merge 2 commits intoIvorySQL:masterIvorySQL/IvorySQL:masterfrom
Conversation
…interval.c - Add buffer size parameter to AppendSeconds function - Replace sprintf with snprintf for safer string formatting - Fix buffer overflow vulnerabilities in interval data type functions Fixes buffer overflow issues in INTERVAL DAY TO SECOND and INTERVAL YEAR TO MONTH data types
📝 WalkthroughWalkthroughAdded buffer-size awareness to interval formatting: replaced internal Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contrib/ivorysql_ora/src/datatype/dsinterval.c (1)
791-829: Fix remaining-buffer calculation;sizeof(str)can underflow and reintroduce overflow risk.
stris achar *, sosizeof(str)is pointer size. Aftercpadvances,sizeof(str) - (cp - str) - 1can underflow to a hugesize_t, allowingsnprintfto overrun the real buffer. It also truncates output whencp == str(size becomes 7/3). Pass the actual buffer size intoEncodeDsintervaland compute remaining from that.🐛 Proposed fix
-static void -EncodeDsinterval(struct pg_tm * tm, fsec_t fsec, int style, char *str, int day_precision, int second_precision) +static void +EncodeDsinterval(struct pg_tm * tm, fsec_t fsec, int style, char *str, + int day_precision, int second_precision, size_t bufsize) { char *cp = str; @@ - snprintf(cp, sizeof(str) - (cp - str) - 1, "%0*d %02d:%02d:", day_precision, mday, hour, min); + snprintf(cp, bufsize - (cp - str), "%0*d %02d:%02d:", day_precision, mday, hour, min); cp += strlen(cp); - AppendSeconds(cp, sec, fsec, second_precision, true, sizeof(str) - (cp - str) - 1); + AppendSeconds(cp, sec, fsec, second_precision, true, bufsize - (cp - str)); @@ -EncodeDsinterval(tm, fsec, INTSTYLE_SQL_STANDARD, buf, day_precision, second_precision); +EncodeDsinterval(tm, fsec, INTSTYLE_SQL_STANDARD, buf, day_precision, second_precision, sizeof(buf));
🤖 Fix all issues with AI agents
In `@contrib/ivorysql_ora/src/datatype/yminterval.c`:
- Around line 390-395: The call to AppendSeconds is using the original bufsize
instead of the remaining space, which can overflow; compute remaining = bufsize
- (cp - str) (ensure it is non-negative) and pass that remaining size to
AppendSeconds(cp, sec, fsec, MAX_INTERVAL_PRECISION, true, remaining) so
AppendSeconds does not write past the end of str; update the invocation in
yminterval.c where snprintf advances cp and replace the final bufsize argument
accordingly.
| snprintf(cp, bufsize - (cp - str), "%c%d-%d %c%d %c%d:%02d:", | ||
| year_sign, abs(year), abs(mon), | ||
| day_sign, abs(mday), | ||
| sec_sign, abs(hour), abs(min)); | ||
| cp += strlen(cp); | ||
| AppendSeconds(cp, sec, fsec, MAX_INTERVAL_PRECISION, true); | ||
| AppendSeconds(cp, sec, fsec, MAX_INTERVAL_PRECISION, true, bufsize); |
There was a problem hiding this comment.
Use remaining buffer size when appending seconds (overflow risk).
Here cp has already advanced, but AppendSeconds is passed the full bufsize, which lets it write past the end of str. This reintroduces the overflow risk.
🐛 Proposed fix
-AppendSeconds(cp, sec, fsec, MAX_INTERVAL_PRECISION, true, bufsize);
+AppendSeconds(cp, sec, fsec, MAX_INTERVAL_PRECISION, true, bufsize - (cp - str));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| snprintf(cp, bufsize - (cp - str), "%c%d-%d %c%d %c%d:%02d:", | |
| year_sign, abs(year), abs(mon), | |
| day_sign, abs(mday), | |
| sec_sign, abs(hour), abs(min)); | |
| cp += strlen(cp); | |
| AppendSeconds(cp, sec, fsec, MAX_INTERVAL_PRECISION, true); | |
| AppendSeconds(cp, sec, fsec, MAX_INTERVAL_PRECISION, true, bufsize); | |
| snprintf(cp, bufsize - (cp - str), "%c%d-%d %c%d %c%d:%02d:", | |
| year_sign, abs(year), abs(mon), | |
| day_sign, abs(mday), | |
| sec_sign, abs(hour), abs(min)); | |
| cp += strlen(cp); | |
| AppendSeconds(cp, sec, fsec, MAX_INTERVAL_PRECISION, true, bufsize - (cp - str)); |
🤖 Prompt for AI Agents
In `@contrib/ivorysql_ora/src/datatype/yminterval.c` around lines 390 - 395, The
call to AppendSeconds is using the original bufsize instead of the remaining
space, which can overflow; compute remaining = bufsize - (cp - str) (ensure it
is non-negative) and pass that remaining size to AppendSeconds(cp, sec, fsec,
MAX_INTERVAL_PRECISION, true, remaining) so AppendSeconds does not write past
the end of str; update the invocation in yminterval.c where snprintf advances cp
and replace the final bufsize argument accordingly.
Use str_size parameter instead of sizeof(str) to properly calculate remaining buffer space, preventing output truncation.
Fix multiple sprintf() buffer overflows in dsinterval.c and yminterval.c
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.