[Libreoffice] [PUSHED, partially] Re: [PATCH] cppcheck "Common realloc mistake"

[Caolán McNamara]

On Sun, 2010-10-31 at 20:51 +0100, Gert Faller wrote:
> Hi,
> 
> a second try.
> Making cppcheck happy is not easy...

realloc_patch_1 looks good for the reallocs part of things.

But I believe cppcheck is basically wrong about the ferr not being
closed, its a false positive I think, i.e. closeErrorFile is always
called before returning and it does a fclose(ferr). cppcheck is
apparently just confused by the complexity there. So I skipped that bit
of the patch, but pushed the rest. Thanks.

> The first file may be ok.
> 
> The second no : but what to do ? loose memory or risk a fail ?

Its somewhat of a quandary, but looking at the callers of
_osl_getFullQualifiedDomainName I see two possible solution.

1. I see that the callers are prepared to take a return of NULL on
failure, so one possible solution is to do the same sort of thing as in
the odk one, except returning NULL if the realloc fails.

e.g. something like

char *pTmp;

pTmp = realloc(pFullQualifiedName, ...)
if (pTmp == NULL)
    free(pFullQualifiedName);
pFullQualifiedName = pTmp;

now pFull is NULL on failure, and the realloced string on success and
things should work out ok if this incredibly unlikely scenario ever
happened, given that the callers check for NULL

2. The other possible solution is to take advantage that in this case I
can see that the string is bring shrunk, so (double-checking man
realloc) on failure "the original block is left untouched". Right before
the realloc a NULL char is entered at the point where the programmer
wants to shrink the string to. So the string is already truncated, the
programmer just want to try and free the now unused space. On fail of
realloc pFullQualified is left alone and that truncated original string
will already give the right results, albeit with a tiny extra memory
overhead, so...

char *pTmp;

pFullQualifiedName[nLengthOfHostName] = '\0';
pTmp = realloc(pFullQualifiedName, nLengthOfHostName + 1);
if (pTmp)
    pFullQualifiedName = pTmp;

Would be good too, seeing as we know we're attempting to free the unused
part of the string that lies after the inserted NULL terminator.

> And in "ure/sal/osl/w32/security.c"
> 
>             DWORD  nInfoBuffer = 512;
>             UCHAR* pInfoBuffer = malloc(nInfoBuffer);
> 
>             while (!GetTokenInformation(hAccessToken, TokenUser,
>                                            pInfoBuffer, nInfoBuffer,
> &nInfoBuffer))
>             {
>                 if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
>                     pInfoBuffer = realloc(pInfoBuffer, nInfoBuffer);
>                 else
>                 {
>                     free(pInfoBuffer);
>                     pInfoBuffer = NULL;
>                     break;
>                 }
>             }
> I'm dumb...

In this case I guess the only thing that can be done on realloc failure
is to return from this with sal_False to indicate failure (after doing
the same free of the original pre-realloc buffer. That'll cascade into a
bit of a ugly and tiring free everything else that's malloced and
un-freed just before that return. Its painful to handle these things
completely in C, C++ is easier to manage them with std::vector or scoped
pointers.

C.