Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential buffer overflow in PyArray_NewFromDescr_int of ctors.c #18939

Closed
Daybreak2019 opened this issue May 7, 2021 · 4 comments · Fixed by #18989
Closed

Potential buffer overflow in PyArray_NewFromDescr_int of ctors.c #18939

Daybreak2019 opened this issue May 7, 2021 · 4 comments · Fixed by #18989
Labels

Comments

@Daybreak2019
Copy link

Daybreak2019 commented May 7, 2021

Reproducing code example:

Snippet:

PyArray_NewFromDescr_int(...., **int nd**,......)
{
    ............... 
    if (descr->subarray) {
        PyObject *ret;
        npy_intp **newdims**[2*NPY_MAXDIMS];
        npy_intp *newstrides = NULL;
        **memcpy**(newdims, dims, nd*sizeof(npy_intp));
        if (strides) {
            newstrides = newdims + NPY_MAXDIMS;
            **memcpy**(newstrides, strides, nd*sizeof(npy_intp));
        }
        ........
}

Error message:

When we run our analysis tool on NumPy, a potential buffer-overflow problem was reported. See details below:
File: numpy/core/src/multiarray/ctors.c
Function: PyArray_NewFromDescr_int
Details in description

Possible call path:
1. array_new -> PyArray_NewFromDescr_int
2. PyArray_Zeros -> PyArray_NewFromDescr_int
3. array_fromfile -> PyArray_FromFile -> PyArray_NewFromDescr_int

NumPy/Python version information:

The main branch of NumPy

@eric-wieser
Copy link
Member

I think this is probably safe at the call sites, but adding an assert to PyArray_NewFromDescr_int would probably be a good idea just to verify that

@Daybreak2019
Copy link
Author

Thanks for your reply.
As the variable "nd" may come from an external module or language component (Python), an assert or boundary check is necessary as your suggestion.

@eric-wieser
Copy link
Member

Yes you're right, PyArray_Zeros is an API function, and does not perform any checking of nd before deferring to PyArray_NewFromDescr_int:

NPY_NO_EXPORT PyObject *
PyArray_Zeros(int nd, npy_intp const *dims, PyArray_Descr *type, int is_f_order)
{
PyArrayObject *ret;
if (!type) {
type = PyArray_DescrFromType(NPY_DEFAULT_TYPE);
}
ret = (PyArrayObject *)PyArray_NewFromDescr_int(

@seberg
Copy link
Member

seberg commented May 9, 2021

It might be a bit hard to abuse, but the check for nd should indeed be moved up here to be on the safe side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants