Fixed some potential null derefs in coreclr#123939
Fixed some potential null derefs in coreclr#123939tpa95 wants to merge 20 commits intodotnet:mainfrom
Conversation
If an error occurs while executing the mprotect function, execution will jump to the error label, where the pRetVal null pointer will be dereferenced. The bug was introduced in commit 835fa74 - prior to this, the pRetVal variable was assigned the value of StartBoundary before calling mprotect.
Added missing checks before calling CdStartField. In CdStartField, if typeHandle.IsNull() == true, the mod argument is dereferenced, which is null in all affected calls.
dotnet-policy-service
bot
added
the
community-contribution
| pRetVal = (void *) StartBoundary; | ||
|
|
There was a problem hiding this comment.
This is affecting the munmap call in error branch.
There was a problem hiding this comment.
Yes. If mprotect fails, we jump to error before pRetVal is assigned, so munmap(pRetVal, MemSize) ends up being called with NULL.
Also, I can instead call munmap((void*)StartBoundary, MemSize) in the error branch.
But in the previous version of the code, before commit 435bff2, pRetVal was initialized before calling mprotect.
There was a problem hiding this comment.
Its not clear to me that calling munmap(NULL, ...) would cause a NULL-deref but it did seem like the intent of the code was to unmap the memory on error. Calling munmap(StartBoundary, ...) might be a clearer way to write that but I'll let @janvorli decide.
There was a problem hiding this comment.
I think the munmap would likely fail with EINVAL and would not dereference NULL. But maybe that if the size is large enough to reach memory mapped by some other mapping, it could cause it to be unmapped. The doc doesn't explicitly specify the behavior.
Also, the memory reserved in this case would leak.
So the fix looks good.
There was a problem hiding this comment.
Hi @janvorli . Would you prefer moving pRetVal = (void*)StartBoundary; before mprotect, or changing the error-path cleanup to call munmap((void*)StartBoundary, MemSize) instead? I can update the PR accordingly.
|
@elinor-fung could you also take a look? |
5 participants
Fixed some potential null pointer dereferences in inspect.cpp and virtual.cpp. The errors were discovered using the Svace static analyzer. I didn't observe any in practice.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Reporter: Pavel Tikhomirov (Tihomirov-P@gaz-is.ru).
Organization: Gazinformservice (resp@gaz-is.ru).