Bug #48237 Error handling in os_mem_alloc_large appears to be incorrect

• Submitted: 22 Oct 18:41
• Modified: 18 Nov 0:37
• Reporter: Mark Callaghan
• Status: Closed
• Category: Server: InnoDB Plugin
• Severity: S3 (Non-critical)
• Version: 5.0,5.1,5.1 plugin
• OS: Linux
• Assigned to: Satya B
• Target Version: 5.1+
• Tags: error, innodb, os_mem_alloc_large
• Triage: Triaged: D2 (Serious)

[22 Oct 18:41] Mark Callaghan

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'

[22 Oct 19:39] Harrison Fisk

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.

[23 Oct 13:10] Michael Izioumtchenko

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

[27 Oct 4:53] Jimmy Yang

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

[4 Nov 10:24] Bugs System

Pushed into 5.1.41 (revid:[e-mail]-20091104092152-qz96bzlf2o1japwc) (version source
revid:[e-mail]-20091103162305-08l4gkeuif2ozsoj) (merge vers: 5.1.41)
(pib:13)

[11 Nov 7:48] Bugs System

Pushed into 6.0.14-alpha (revid:[e-mail]-20091110093407-rw5g8dys2baqkt67) (version
source revid:[e-mail]-20091109080109-7dxapd5y5pxlu08w) (merge vers: 6.0.14-alpha)
(pib:13)

[11 Nov 7:56] Bugs System

Pushed into 5.5.0-beta (revid:[e-mail]-20091109115615-nuohp02h8mdrz8m2) (version
source revid:[e-mail]-20091105122958-jyqjx9xus8v4e0yd) (merge vers: 5.5.0-beta)
(pib:13)

[18 Nov 0:38] Paul DuBois

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.