[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: webkit-1.8.0-2: Patches for review



Hi,

(interesting you sent your initial patch, with no changes...)

Alle lunedì 16 aprile 2012, Svante Signell ha scritto:
> Problem with the patch is that the allocated string cannot easily be
> freed:

Sure it can.

> The string str is declared const char* and I don't want to change
> that definition. Maybe leaving the string allocated won't cause any
> problems?

Like memory leaks? No please.

> @@ -40,9 +42,14 @@
>  #elif OS(UNIX)
>  CString getCurrentExecutablePath()
>  {
> -    static char readLinkBuffer[PATH_MAX];
> -    ssize_t result = readlink("/proc/curproc/file", readLinkBuffer, PATH_MAX);
> -    if (result == -1)
> +    struct stat sb;
> +    char *readLinkBuffer;
> +    ssize_t result;
> +    result = lstat("/proc/curproc/file", &sb);
> +    readLinkBuffer = (char*)malloc(sb.st_size + 1);
> +    if (readLinkBuffer == NULL) return CString();

(I guess this if (..) return should be indented like the rest of the
code.)

> +    result = readlink("/proc/curproc/file", readLinkBuffer, sb.st_size + 1);

So far ok up to this...

> +    if (result == -1 || result > sb.st_size)
>          return CString();
>      return CString(readLinkBuffer, result);

... but then you leak readLinkBuffer in both the cases (error return
value from readlink(), and success); you can do something like this:

| CString resultString;
| if (result >= 0 && result < sb.st_size)
|   resultString = CString(readLinkBuffer, result);
| free(readLinkBuffer);
| return resultString;

note in this case the CString constructor does not need the \0 byte
at the end of the buffer (since it takes the char count), but take care
you should have elsewhere done like
  readLinkBuffer[result] = 0;

-- 
Pino Toscano

Attachment: signature.asc
Description: This is a digitally signed message part.


Reply to: