Hello Daniel,
On 10/28/19 5:12 PM, Daniel Kiper wrote:
> On Mon, Oct 28, 2019 at 02:11:10PM +0100, Javier Martinez Canillas wrote:
>> From: Peter Jones <pjo...@redhat.com>
[snip]
>> grub_util_create_envblk_file (const char *name)
>> {
>> + int rc;
>> FILE *fp;
>> char *buf;
>> char *namenew;
>> + ssize_t size = 1;
>> + char *rename_target = xstrdup (name);
>
> What will happen if rename_target is NULL?
>
I've looked at this and it's not possible for rename_target to be NULL since
xstrdup() calls xmalloc() to allocate memory and this function already checks
if the returned pointer is NULL and calls grub_util_error() if that's the case
which exits after printing an error message.
So the code is correct and in fact I see that all xstrdup() callers (or for
any other x* function that makes allocations) don't check the return value.
>> buf = xmalloc (DEFAULT_ENVBLK_SIZE);
>>
>> @@ -60,7 +64,58 @@ grub_util_create_envblk_file (const char *name)
>> free (buf);
>> fclose (fp);
>>
>> - if (grub_util_rename (namenew, name) < 0)
>> - grub_util_error (_("cannot rename the file %s to %s"), namenew, name);
>> + while (1)
>> + {
>> + char *linkbuf;
>> + ssize_t retsize;
>> +
>> + linkbuf = xmalloc (size+1);
>
> Please check for NULL here. And "size + 1". In general I will not accept
Ok, will add spaces around the + operator.
> any patch which does not check for NULL for malloc() et consortes.
>
Same, all callers of xmalloc() don't check this since as mentioned the function
already takes care of this.
>> + retsize = grub_util_readlink (rename_target, linkbuf, size);
>> + if (retsize < 0 && (errno == ENOENT || errno == EINVAL))
>> + {
>> + free (linkbuf);
>> + break;
>> + }
>> + else if (retsize < 0)
>> + {
>> + grub_util_error (_("cannot rename the file %s to %s: %m"), namenew,
>> name);
>> + free (linkbuf);
>> + free (namenew);
>> + return;
>> + }
>> + else if (retsize == size)
>> + {
>> + free (linkbuf);
>> + size += 128;
>> + continue;
>> + }
>> +
>> + linkbuf[retsize] = '\0';
>> + if (linkbuf[0] == '/')
>> + {
>> + free (rename_target);
>> + rename_target = linkbuf;
>> + }
>> + else
>> + {
>> + char *dbuf = xstrdup (rename_target);
>
> What happens if dbuf is NULL?
>
Same than above.
>> + const char *dir = dirname (dbuf);
>
> Empty line please...
>
Ok.
>> + free (rename_target);
>> + rename_target = xasprintf ("%s/%s", dir, linkbuf);
>
> NULL?
>
Same than above.
> Daniel
>
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
