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

lib: fix a Null Pointer Dereference bug. #18066

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mugitya03
Copy link

The function elfsect_wrap can return a NULL value when allocation fails at line 573.

	w = (struct elfsect *)typeobj_elfsect.tp_alloc(&typeobj_elfsect, 0);
	if (!w)
		return NULL;

The null value returned by function elfsect_wrap is saved to w->sects[idx] and further assigned to pointer ret. Then the pointer ptr is passed to function Py_INCREF at line , where it can be dereferenced.

		w->sects[idx] = elfsect_wrap(w, scn, idx, name);
	}

	ret = w->sects[idx];
	Py_INCREF(ret);
	return ret;

The definition of function Py_INCREF can be found here: link

uint32_t local = _Py_atomic_load_uint32_relaxed(&op->ob_ref_local);

Thus, we add a null value check before calling the function Py_INCREF.

@Jafaral
Copy link
Member

Jafaral commented Feb 8, 2025

@Mergifyio backport dev/10.3 stable/10.2 stable/10.1 stable/10.0

Copy link

mergify bot commented Feb 8, 2025

backport dev/10.3 stable/10.2 stable/10.1 stable/10.0

🟠 Waiting for conditions to match

  • merged [📌 backport requirement]

@@ -693,7 +693,8 @@ static PyObject *elffile_secbyidx(struct elffile *w, Elf_Scn *scn, size_t idx)
}

ret = w->sects[idx];
Py_INCREF(ret);
if (ret)
Py_INCREF(ret);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather replace to Py_XINCREF all the occurrences.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Py_XINCREF first checks for a null value. I'll submit a new PR replacing the API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually not correct as CPython functions are not allowed to return NULL without setting an error. This change coverts a guaranteed crash into indeterminate Python behavior.

@eqvinox
Copy link
Contributor

eqvinox commented Feb 11, 2025

This work is completely irrelevant since this code is only executed at build time in a controlled environment.

@eqvinox eqvinox removed the backport label Feb 11, 2025
@FRRouting FRRouting deleted a comment from mergify bot Feb 11, 2025
Copy link
Contributor

@eqvinox eqvinox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to just NAK this since this is a build-time only tool and if we run out of memory here the build has failed anyway. Considering how memory allocation works, the failure behavior is not sane in any case as the build will start paging and swapping before this happens. Whether we ultimately crash with a SEGV or something nicer is… academic.

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

Successfully merging this pull request may close these issues.

4 participants