MySQL Bugs: #48237: Error handling in os_mem_alloc

Bug #48237	Error handling in os_mem_alloc_large appears to be incorrect
Submitted:	22 Oct 2009 16:41	Modified:	19 Jun 2010 0:18
Reporter:	Mark Callaghan	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: InnoDB Plugin storage engine	Severity:	S3 (Non-critical)
Version:	5.0,5.1,5.1 plugin	OS:	Linux
Assigned to:	Satya B	CPU Architecture:	Any
Tags:	error, innodb, os_mem_alloc_large

Description:
Error handling in os_mem_alloc_large for failures from shmat() appears to be wrong as code that follows doesn't check for 'ptr == (void*) -1'. When it fails and 'ptr == (void*) -1', then:
* shmctl will still be called
* the 'if (ptr)' block will be entered

Code from os_mem_alloc_large in os/os0proc.c
 
        shmid = shmget(IPC_PRIVATE, (size_t)size, SHM_HUGETLB | SHM_R | SHM_W);
        if (shmid < 0) {
                fprintf(stderr, "InnoDB: HugeTLB: Warning: Failed to allocate"
                        " %lu bytes. errno %d\n", size, errno);
                ptr = NULL;
        } else {
                ptr = shmat(shmid, NULL, 0);
                if (ptr == (void *)-1) {
                        fprintf(stderr, "InnoDB: HugeTLB: Warning: Failed to"
                                " attach shared memory segment, errno %d\n",
                                errno);
                }

                /* Remove the shared memory segment so that it will be
                automatically freed after memory is detached or
                process exits */
                shmctl(shmid, IPC_RMID, &buf);
        }

        if (ptr) {
                *n = size;
                os_fast_mutex_lock(&ut_list_mutex);
                ut_total_allocated_memory += size;
                os_fast_mutex_unlock(&ut_list_mutex);
# ifdef UNIV_SET_MEM_TO_ZERO
                memset(ptr, '\0', size);
# endif
                UNIV_MEM_ALLOC(ptr, size);
                return(ptr);
        }

How to repeat:
review the code

Suggested fix:
check for 'ptr == (void*) -1'

This is primarily a problem with the InnoDB plugin since more work is done in the if (ptr) block, however it does affect earlier version of InnoDB as well.

Note that currently, this is only called if you use the --large-pages start-up option.

It is a relatively trivial, unrisky fix to do imo.

Jimmy, could you have a look.
Mark, we still need to call shmctl() to remove the shared memory segment if shmat() fails. My proposed fiz therefore is to add

ptr = NULL;

after that shmctl to avoid entering if (ptr) block

Agreed. In case of shmat() failure, shmctl() with IPC_RMID still needs to be called, and 'if (ptr)' block should be skipped and fall through to ut_malloc_low(). Good catch.

Thanks
Jimmy

Pushed into 5.1.41 (revid:joro@sun.com-20091104092152-qz96bzlf2o1japwc) (version source revid:kristofer.pettersson@sun.com-20091103162305-08l4gkeuif2ozsoj) (merge vers: 5.1.41) (pib:13)

Pushed into 6.0.14-alpha (revid:alik@sun.com-20091110093407-rw5g8dys2baqkt67) (version source revid:alik@sun.com-20091109080109-7dxapd5y5pxlu08w) (merge vers: 6.0.14-alpha) (pib:13)

Pushed into 5.5.0-beta (revid:alik@sun.com-20091109115615-nuohp02h8mdrz8m2) (version source revid:svoj@sun.com-20091105122958-jyqjx9xus8v4e0yd) (merge vers: 5.5.0-beta) (pib:13)

Noted in 5.1.41, 5.5.0, 6.0.14 changelogs.

Memory-allocation failures were handled incorrectly in the InnoDB
os_mem_alloc_large() function.

Pushed into 5.1.41-ndb-7.1.0 (revid:jonas@mysql.com-20091218102229-64tk47xonu3dv6r6) (version source revid:jonas@mysql.com-20091218095730-26gwjidfsdw45dto) (merge vers: 5.1.41-ndb-7.1.0) (pib:15)

Pushed into 5.1.41-ndb-6.2.19 (revid:jonas@mysql.com-20091218100224-vtzr0fahhsuhjsmt) (version source revid:jonas@mysql.com-20091217101452-qwzyaig50w74xmye) (merge vers: 5.1.41-ndb-6.2.19) (pib:15)

Pushed into 5.1.41-ndb-6.3.31 (revid:jonas@mysql.com-20091218100616-75d9tek96o6ob6k0) (version source revid:jonas@mysql.com-20091217154335-290no45qdins5bwo) (merge vers: 5.1.41-ndb-6.3.31) (pib:15)

Pushed into 5.1.41-ndb-7.0.11 (revid:jonas@mysql.com-20091218101303-ga32mrnr15jsa606) (version source revid:jonas@mysql.com-20091218064304-ezreonykd9f4kelk) (merge vers: 5.1.41-ndb-7.0.11) (pib:15)

Pushed into 5.1.47 (revid:joro@sun.com-20100505145753-ivlt4hclbrjy8eye) (version source revid:vasil.dimov@oracle.com-20100331130613-8ja7n0vh36a80457) (merge vers: 5.1.46) (pib:16)

Push resulted from incorporation of InnoDB tree. No changes pertinent to this bug.
Re-closing.

Pushed into mysql-next-mr (revid:alik@sun.com-20100524190136-egaq7e8zgkwb9aqi) (version source revid:vasil.dimov@oracle.com-20100331130613-8ja7n0vh36a80457) (pib:16)

Pushed into 6.0.14-alpha (revid:alik@sun.com-20100524190941-nuudpx60if25wsvx) (version source revid:vasil.dimov@oracle.com-20100331130613-8ja7n0vh36a80457) (merge vers: 5.1.46) (pib:16)

Pushed into 5.5.5-m3 (revid:alik@sun.com-20100524185725-c8k5q7v60i5nix3t) (version source revid:vasil.dimov@oracle.com-20100331130613-8ja7n0vh36a80457) (merge vers: 5.1.46) (pib:16)

Push resulted from incorporation of InnoDB tree. No changes pertinent to this bug.
Re-closing.

Pushed into 5.5.5-m3 (revid:alik@sun.com-20100615080459-smuswd9ooeywcxuc) (version source revid:mmakela@bk-internal.mysql.com-20100415070122-1nxji8ym4mao13ao) (merge vers: 5.1.47) (pib:16)

Pushed into mysql-next-mr (revid:alik@sun.com-20100615080558-cw01bzdqr1bdmmec) (version source revid:mmakela@bk-internal.mysql.com-20100415070122-1nxji8ym4mao13ao) (pib:16)

Pushed into 5.1.47-ndb-7.0.16 (revid:martin.skold@mysql.com-20100617114014-bva0dy24yyd67697) (version source revid:vasil.dimov@oracle.com-20100331130613-8ja7n0vh36a80457) (merge vers: 5.1.46) (pib:16)

Pushed into 5.1.47-ndb-6.2.19 (revid:martin.skold@mysql.com-20100617115448-idrbic6gbki37h1c) (version source revid:vasil.dimov@oracle.com-20100331130613-8ja7n0vh36a80457) (merge vers: 5.1.46) (pib:16)

Pushed into 5.1.47-ndb-6.3.35 (revid:martin.skold@mysql.com-20100617114611-61aqbb52j752y116) (version source revid:vasil.dimov@oracle.com-20100331130613-8ja7n0vh36a80457) (merge vers: 5.1.46) (pib:16)