You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by Ou Changkun <ou...@outlook.com> on 2018/09/18 18:58:10 UTC
guac_user_alloc() can fail but not checked?
Hi Guacamole developers,
I just noticed a unclear code behavior in
guacamole-server/src/libguac/user.c:
=====
(….)
guac_user* user = calloc(1, sizeof(guac_user));
int i;
/* Generate ID */
user->user_id = guac_generate_id(GUAC_USER_ID_PREFIX);
(….)
=====
Still in master branch: https://github.com/apache/guacamole-server/blob/332e187813595fc2e769f3e29c0582b7ec726ea1/src/libguac/user.c#L41
Further, its caller also not verify if guac_user_alloc() returns NULL user:
=====
/* Create skeleton user */
guac_user* user = guac_user_alloc();
user->socket = socket;
user->client = client;
user->owner = params->owner;
=====
Location: https://github.com/apache/guacamole-server/blob/67680bd2d51e7949453f0f7ffc7f4234a1136715/src/guacd/proc.c#L92
I am wondering weather this is intentional or not?
Should the `calloc` call be verified if returns NULL pointer?
It seems accessing NULL struct pointer members is an
undefined behavior? Am I missing something here?
Best,
Changkun
Re: guac_user_alloc() can fail but not checked?
Posted by Mike Jumper <mj...@apache.org>.
On Tue, Sep 18, 2018 at 12:51 PM, Ou Changkun <ou...@outlook.com>
wrote:
> Hi Mike,
>
> Thanks for your confirmation. I found some related case:
> https://stackoverflow.com/questions/13245312/calloc-fails-
> and-returns-null/13252245
>
> > calloc fails even there is enough memory.
>
> Perhaps may also occur in some restricted docker container runtime? Is it
> something should care about? Any visions?
>
>
Going through the code and making sure all malloc() / calloc() / realloc()
calls are properly checked for NULL, and making sure any of the guac_*()
functions which may return NULL are also similarly checked, would be a very
welcome change. Daunting, but welcome. ;)
There may be static analysis tools that can assist.
- Mike
Re: guac_user_alloc() can fail but not checked?
Posted by Ou Changkun <ou...@outlook.com>.
Hi Mike,
Thanks for your confirmation. I found some related case: https://stackoverflow.com/questions/13245312/calloc-fails-and-returns-null/13252245
> calloc fails even there is enough memory.
Perhaps may also occur in some restricted docker container runtime? Is it something should care about? Any visions?
Best,
Changkun
> On Sep 18, 2018, at 21:35, Mike Jumper <mj...@apache.org> wrote:
>
> On Tue, Sep 18, 2018 at 11:58 AM, Ou Changkun <ou...@outlook.com>
> wrote:
>
>> Hi Guacamole developers,
>>
>> I just noticed a unclear code behavior in
>> guacamole-server/src/libguac/user.c:
>>
>> =====
>> (….)
>> guac_user* user = calloc(1, sizeof(guac_user));
>> int i;
>>
>> /* Generate ID */
>> user->user_id = guac_generate_id(GUAC_USER_ID_PREFIX);
>> (….)
>> =====
>>
>> Still in master branch: https://github.com/apache/guac
>> amole-server/blob/332e187813595fc2e769f3e29c0582b7ec726ea1/s
>> rc/libguac/user.c#L41
>>
>> Further, its caller also not verify if guac_user_alloc() returns NULL user:
>>
>> =====
>> /* Create skeleton user */
>> guac_user* user = guac_user_alloc();
>> user->socket = socket;
>> user->client = client;
>> user->owner = params->owner;
>> =====
>>
>> Location: https://github.com/apache/guacamole-server/blob/67680bd2d51e
>> 7949453f0f7ffc7f4234a1136715/src/guacd/proc.c#L92
>>
>> I am wondering weather this is intentional or not?
>> Should the `calloc` call be verified if returns NULL pointer?
>>
>
> It would be good practice to always check calls to malloc() and calloc(),
> yes.
>
> This isn't done 100% across the board mainly because OS kernels commonly
> use an opportunistic allocation strategy. In those cases, malloc() and
> friends will always return non-NULL, and thus software which religiously
> checks for NULL will still crash / be killed if such a system is under
> sufficient memory pressure.
>
> It seems accessing NULL struct pointer members is an
>> undefined behavior? Am I missing something here?
>>
>
> It would be better to check for NULL.
>
> Lacking that check, the result isn't undefined - a segfault will occur upon
> the attempt to dereference NULL.
>
> - Mike
Re: guac_user_alloc() can fail but not checked?
Posted by Mike Jumper <mj...@apache.org>.
On Tue, Sep 18, 2018 at 11:58 AM, Ou Changkun <ou...@outlook.com>
wrote:
> Hi Guacamole developers,
>
> I just noticed a unclear code behavior in
> guacamole-server/src/libguac/user.c:
>
> =====
> (….)
> guac_user* user = calloc(1, sizeof(guac_user));
> int i;
>
> /* Generate ID */
> user->user_id = guac_generate_id(GUAC_USER_ID_PREFIX);
> (….)
> =====
>
> Still in master branch: https://github.com/apache/guac
> amole-server/blob/332e187813595fc2e769f3e29c0582b7ec726ea1/s
> rc/libguac/user.c#L41
>
> Further, its caller also not verify if guac_user_alloc() returns NULL user:
>
> =====
> /* Create skeleton user */
> guac_user* user = guac_user_alloc();
> user->socket = socket;
> user->client = client;
> user->owner = params->owner;
> =====
>
> Location: https://github.com/apache/guacamole-server/blob/67680bd2d51e
> 7949453f0f7ffc7f4234a1136715/src/guacd/proc.c#L92
>
> I am wondering weather this is intentional or not?
> Should the `calloc` call be verified if returns NULL pointer?
>
It would be good practice to always check calls to malloc() and calloc(),
yes.
This isn't done 100% across the board mainly because OS kernels commonly
use an opportunistic allocation strategy. In those cases, malloc() and
friends will always return non-NULL, and thus software which religiously
checks for NULL will still crash / be killed if such a system is under
sufficient memory pressure.
It seems accessing NULL struct pointer members is an
> undefined behavior? Am I missing something here?
>
It would be better to check for NULL.
Lacking that check, the result isn't undefined - a segfault will occur upon
the attempt to dereference NULL.
- Mike