0

I have a code like this:

std::string getInfo(FILE *fp)
{
    char buffer[30];
    if (fread(buffer, 19, 1, fp) == 1)
        buffer[19] = '\0';
    else
        buffer[0] = '\0';

    return buffer;
}

I'm using cppcheck for static analysis and it spits a warning:

error: Resource leak: fp [resourceLeak]
 return buffer;
 ^

The way I see it, since the return is by value, the data will be copied from "buffer" into std::string's storage, so there's no leak there.

Does this pose some real problem or is it a false positive?

einpoklum
  • 118,144
  • 57
  • 340
  • 684
Milan Babuškov
  • 59,775
  • 49
  • 126
  • 179
  • 7
    It looks like it's complaining about your file pointer and not the buffer or std::string. – Voo Apr 12 '22 at 15:13
  • @Voo: which still is presumably the caller's responsibility to `fclose` – Ben Voigt Apr 12 '22 at 15:13
  • 1
    However, this code does look buggy. It's not a good idea to have the `std::string` constructor just figure out the length using `strlen` when you already know the amount of data read from the file. Or is it your intention to trim at the first NUL byte (but still read a full 19 bytes from the file advancing the internal read pointer)? A documentation comment would go a long way. – Ben Voigt Apr 12 '22 at 15:15
  • 1
    @BenVoigt Which may or may not be happening in his program. We can't see that. I would assume cppcheck can and is. – Taekahn Apr 12 '22 at 15:24
  • Maybe related: https://stackoverflow.com/q/17077907/10871073 If that's the only place in the code where `fp` is used and it is *not* closed in the calling module, then maybe cppcheck reports the leak there? – Adrian Mole Apr 12 '22 at 15:25
  • @Ben Sure, my point was that it's the parameter the OP should focus on, since the std::string/buffer code will not cause any leak. It's possible that the passed in file pointer isn't released from the caller for some reason.. – Voo Apr 12 '22 at 15:27
  • 1
    Milan, please expand this into a proper example: A program which cppcheck complains about, not just a function. – einpoklum Apr 12 '22 at 15:27
  • Good catch. I was reading through so much output, I completely missed the "fp" part. Yeah, there's fopen with missing fclose. Thanks. – Milan Babuškov Apr 12 '22 at 15:29

2 Answers2

9

error: Resource leak: fp [resourceLeak]

You are not leaking any resource from this function. So, if you're only analyzing this function, this is a bug in your cppcheck. However, as @Taekahn points out - maybe the leak is elsewhere in your program.

Also, since you're returning std::string, you can use a constructor taking a pointer and a count, and not bother with the '\0' in the buffer:

std::string getInfo(FILE *fp)
{
    static constexpr const std::size_t info_length { 19 };
    char buffer[info_length];
    if (fread(buffer, info_length, 1, fp) == 1)
        return {buffer, info_length};
    else
        return {};
}

If you're using C++23, you might consider returning an std::expected instead of the empty string, e.g. something like:

std::expected<std::string, errc> getInfo(FILE *fp)
{
    static constexpr const std::size_t info_length { 19 };
    char buffer[info_length];
    if (fread(buffer, info_length, 1, fp) == 1)
        return std::string{buffer, info_length};
    else
        return std::unexpected{errno};
}
einpoklum
  • 118,144
  • 57
  • 340
  • 684
  • @463035818_is_not_a_number: Sorry, I missed that. – einpoklum Apr 12 '22 at 15:20
  • Maybe he is leaking the resource. We can't see the rest of the program. cppcheck can. – Taekahn Apr 12 '22 at 15:23
  • 2
    Not an answer, just a guess. Maybe OP calls `getInfo(fopen(path));` and does not show it in a [mcve]. – 273K Apr 12 '22 at 15:25
  • @273K: That's possible. I'd make this comment on the question to get OP's attention though. – einpoklum Apr 12 '22 at 15:26
  • Just to confirm, the issue was a missing fclose(). I was so focused on string memory that I missed the "fp" in the cppcheck output. Thanks! – Milan Babuškov Apr 12 '22 at 15:38
  • @MilanBabuškov It's unbelievable, you, who has reached 57.3k scores, did not post a [mcve] like a newbie on SO and caused other users blaming cppcheck. – 273K Apr 12 '22 at 15:48
  • 1
    @273K: It's not unbelievable, because Milan's post insinuated that cppcheck was complaining about _this function per se_, so that it was an MRE. – einpoklum Apr 12 '22 at 15:57
3

There is no leak in the shown function.

The std::FILE* provided as the argument may have been created using std::fopen, and such resource may leak if it isn't closed at some point. But since this function doesn't acquire the resource, this function shouldn't be responsible for releasing it either.

In conclusion, it's either a false positive or misleading diagnostic. At best (from point of view of the diagnostic), there may be a leak somewhere else, although that's not demonstrated within the example.

P.S. Since you return a std::string, you can avoid copying the content first into a separate buffer by reading directly into the std::string:

constexpr std::size_t count = 19;
std::string buffer(count, '\0');
std::size_t r = fread(buffer.data(), count, 1, fp);
if (r)
    return buffer;
else
    return {};
eerorika
  • 232,697
  • 12
  • 197
  • 326