3 ms·
Spot the bug in the following C function for reading files: bool files_read(const char *path, char **data, size_t *size) { FILE *f = fopen(path
by bjourne 3y ago
Spot the bug in the following C function for reading files:
bool
files_read(const char *path, char **data, size_t *size) {
FILE *f = fopen(path, "rb");
if (!f) {
return false;
}
fseek(f, 0, SEEK_END);
size_t n = (size_t)ftell(f);
rewind(f);
if (size) {
*size = n;
}
if (!data) {
goto ok;
}
*data = (char*)malloc(sizeof(char)*(n + 1));
if (fread(*data, 1, n, f) != n) {
free(*data);
return false;
}
*data[n] = '\0';
ok:
fclose(f);
return true;
}
ChatGPT solves this task instantly. Many, even experienced C developers, would have trouble finding the bug. And the lack of error checking on malloc is not a bug.
- golergka 3y agoJust for reference, copied this snippet in GPT-4, it generated this answer in 1,5 seconds: > The issue in this code is related to memory access. The line data[n] = '\0'; should be (data)[n] = '\0';. And then went on to generate corrected code and this explanation: > The original expression, data[n], first applies the array subscript operation, which is equivalent to (data + n), and then tries to dereference the resulting pointer, which is not the intention here. This will likely lead to a segmentation fault because data is a pointer to pointer, and data[n] will try to access memory that has not been allocated. > The corrected expression, (*data)[n], first dereferences data to get the allocated char array, and then applies the array subscript operation to set the null terminator at the correct location.
- ChatGTP 3y agoJust wondering if you think this type of bug hasn't been covered at least 50x in many online forums, I'd be very surprised? I'm not trying to say this isn't impressive, but seems fairly obvious and more like something a linter / analyzer would pickup ?
- almog 3y agoEven if this isn't based on similar questions that can be found online, a bug in existing systems usually wouldn't introduce itself by framing an isolated section of code. More often than not it'd be through indirection or a side effect that covers a larger execution path.
- deleted 3y ago[deleted]
- golergka 3y agoTo an experienced C developer who works with the language constantly — probably. I'm not one, so I wouldn't know. I've found GPT to be much more helpful when you're working with technologies you'r not as familiar or up to date with.
- thfuran 3y agoWhat does it do if you correct the bug and then ask it what the bug is?
- pfdietz 3y agoYou also forgot to close f when returning false.
- deleted 3y ago[deleted]
- Joker_vD 3y agoNot going to talk about the actual bug(s), but... why is it trying to NUL-terminate the buffer full of binary ("rb") data? Also, sizeof(char)? That's 1 by definition. Actually, no, let's talk about the bug. What's with people's obsession of eliminating local temporaries and working straight with pointer-to-result? char *buff = malloc(n + 1); if (fread(buff, 1, n, f) != n) { free(buff); fclose(f); return false; } buff[n] = '\0'; *data = buff; ok: fclose(f); return true; This way, data won't contain a freed pointer when reading fails.
- bjourne 3y ago"rb" because you don't want Windows to open the file in translated mode and munging its contents. Null-termination so that the same code can deal with both binary and text files. And I prefer all mallocs to follow the pattern malloc(sizeof(tp)*n) because it's less hassle than thinking about what types' sizes may vary. This rabbit hole is endless and we haven't even started covering my use of fseek/ftell/rewind!
- Joker_vD 3y agoYeah, "fread(buff, 1, result_from_ftell, f)" can have some pretty strange effects on the buff's content if CHAR_BIT != 8.