Code base Improvement #2125
Conversation
|
To improve the given Base64 encoding and decoding code, consider the following enhancements: Reduce redundancy and simplify conditions. |
1 similar comment
|
To improve the given Base64 encoding and decoding code, consider the following enhancements: Reduce redundancy and simplify conditions. |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the Base64 encoding and decoding implementation to improve code quality by simplifying logic, using modern C features (stdbool.h), and renaming functions for clarity.
Changes:
- Replaced TRUE/FALSE macros with stdbool.h and updated is_base64 to return bool
- Renamed internal functions from encode/decode to encode_char/decode_char for better clarity
- Simplified encoding and decoding logic using ternary operators and inline variable initialization
- Changed encode_base64 parameter to const char* to indicate it doesn't modify input
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| *p++ = (i + 1 < size) ? encode_char(((b2 & 0xf) << 2) | (b3 >> 6)) : '='; | ||
| *p++ = (i + 2 < size) ? encode_char(b3 & 0x3f) : '='; | ||
| } | ||
|
|
There was a problem hiding this comment.
The null terminator is not explicitly written to the output buffer. While calloc initializes memory to zero (which provides a null terminator), after the loop completes, the pointer p has advanced beyond the last written character but the null terminator position is not guaranteed. For Base64 encoding, the output should always be a multiple of 4 characters (with padding), so the calculation (size + 2) / 3 * 4 + 1 should be correct, but it would be more explicit and maintainable to write *p = '\0'; after the loop to document the intent and ensure null termination regardless of calloc behavior.
| *p = '\0'; |
|
|
||
| return 63; | ||
| } | ||
|
|
There was a problem hiding this comment.
Similar to encode_base64, the null terminator is not explicitly written after decoding completes. While calloc initializes the buffer to zero, explicitly writing *p = '\0'; after the loop would make the null termination guarantee more obvious and maintainable. This is especially important for decode_base64 since the output length varies based on padding.
| *p = '\0'; |
| b2 = src[i + 1]; | ||
| } | ||
| /* Encode data to Base64 */ | ||
| char *encode_base64(int size, const char *src) { |
There was a problem hiding this comment.
The function signature has been changed to use const char *src parameter, but the header files at src/addagent/manage_agents.h:18 and src/win32/ui/os_win32ui.h:50 still declare it with char *src (non-const). This creates a signature mismatch between the declaration and definition. You should update the header files to match this new signature with const char *src, otherwise this may cause compilation warnings or errors depending on the compiler settings.
| if (buf[k + 2] != '=') *p++ = ((b2 & 0xf) << 4) | (b3 >> 2); | ||
| if (buf[k + 3] != '=') *p++ = ((b3 & 0x3) << 6) | b4; |
There was a problem hiding this comment.
The padding checks on lines 119-120 access buf[k + 2] and buf[k + 3] without verifying these indices are within bounds. If the filtered buffer length l is not a multiple of 4, these accesses could read beyond the allocated buffer, causing undefined behavior. You should add bounds checks: if (k + 2 < l && buf[k + 2] != '=') and if (k + 3 < l && buf[k + 3] != '=') before accessing these elements.
| if (buf[k + 2] != '=') *p++ = ((b2 & 0xf) << 4) | (b3 >> 2); | |
| if (buf[k + 3] != '=') *p++ = ((b3 & 0x3) << 6) | b4; | |
| if (k + 2 < l && buf[k + 2] != '=') *p++ = ((b2 & 0xf) << 4) | (b3 >> 2); | |
| if (k + 3 < l && buf[k + 3] != '=') *p++ = ((b3 & 0x3) << 6) | b4; |
| int out_len = len * 3 / 4; | ||
| char *dest = (char *)calloc(out_len + 1, sizeof(char)); // +1 for null terminator |
There was a problem hiding this comment.
The memory allocation calculation len * 3 / 4 may be insufficient for the decoded output. This calculation assumes the input length is a multiple of 4, but after filtering non-Base64 characters, the actual valid Base64 string length may differ. The allocation should be based on the filtered length l after line 108, not the original input length. This could lead to buffer overflow when writing decoded bytes. Consider moving the allocation after filtering, or allocate a safe upper bound like len bytes.
| static unsigned char decode_char(char c); | ||
|
|
||
| /* Global variables */ | ||
| /* Global functions */ |
There was a problem hiding this comment.
The comment says "Global functions" but these are function prototypes/declarations, not global variables. While technically functions have global scope by default, the more conventional comment would be "Function prototypes" or "Public API" since these appear to be the public interface functions. The original comment "Global variables" was indeed incorrect, but "Global functions" is still somewhat misleading.
| /* Global functions */ | |
| /* Public API (function prototypes) */ |
To improve the given Base64 encoding and decoding code, consider the following enhancements:
Reduce redundancy and simplify conditions.
Use consistent and meaningful naming conventions.
Add error handling and parameter validation.
Optimize memory allocation.