[NTOBJSHEX] Return early from GetInfoFromPidl when pcidl is null in LPCITEMIDLIST #4946

muthu90tech · 2022-12-18T08:32:09Z

Purpose

This fixes crash when attempting to drag n drop items from NTObject Namespace and System Registry.

Do a quick recap of your work here.

JIRA issues: CORE-18480, CORE-18481

Proposed changes

Perform a null check in GetInfoFromPidel on mkid in LPCITEMIDLIST

avoid_drag_drop_crash.mp4

(refer video, no more crash)

LPCITEMIDLIST, this fixes crash when attempting to drag n drop items from NTObject Namespace and System Registry.

dll/shellext/ntobjshex/ntobjfolder.cpp

ghost · 2022-12-18T12:48:45Z

lgtm as based on my patch.

learn-more

This is a very unreadable way of checking pcidl for null, please do that instead.

JoachimHenze · 2022-12-18T14:58:49Z

This is a very unreadable way of checking pcidl for null, please do that instead.

?
Wouldn't he need to check "pcidl->mkid" for being NULL instead?
If pcidl was indeed NULL, then his code would not work, but crash due to null-dereference already. Which is obviously not the case.

If he was to check something else than entry, then it would also make sense to move up the check.

But honestly I do not really understand your request yet, Mark.

learn-more · 2022-12-21T08:07:58Z

This is a very unreadable way of checking pcidl for null, please do that instead.

? Wouldn't he need to check "pcidl->mkid" for being NULL instead? If pcidl was indeed NULL, then his code would not work, but crash due to null-dereference already.

No, checking pcidl->mkid is also wrong if pcidl is null.

Which is obviously not the case.

Think again.

If he was to check something else than entry, then it would also make sense to move up the check.

indeed.

But honestly I do not really understand your request yet, Mark.

I am unsure what is unclear about 'xxx' is an unreadable way of doing 'yyy', please do 'yyy' instead.

If the pointer math is confusing to you, try it out with a char* array on something like https://godbolt.org/

muthu90tech · 2022-12-21T08:21:37Z

This is a very unreadable way of checking pcidl for null, please do that instead.

? Wouldn't he need to check "pcidl->mkid" for being NULL instead? If pcidl was indeed NULL, then his code would not work, but crash due to null-dereference already.

No, checking pcidl->mkid is also wrong if pcidl is null.

The GetInfoFromPidl(pidl, &info) gets called only if pidl in not null. Please see: https://github.com/reactos/reactos/blob/master/dll/shellext/ntobjshex/regfolder.cpp#L191

So pcidl in GetInfoFromPidl can not be null.

learn-more · 2022-12-21T10:21:57Z

This is a very unreadable way of checking pcidl for null, please do that instead.

? Wouldn't he need to check "pcidl->mkid" for being NULL instead? If pcidl was indeed NULL, then his code would not work, but crash due to null-dereference already.

No, checking pcidl->mkid is also wrong if pcidl is null.

The GetInfoFromPidl(pidl, &info) gets called only if pidl in not null. Please see: https://github.com/reactos/reactos/blob/master/dll/shellext/ntobjshex/regfolder.cpp#L191

You have checked just one invocation:
https://git.reactos.org/?p=reactos.git&a=search&h=HEAD&st=grep&sr=1&s=GetInfoFromPidl

So pcidl in GetInfoFromPidl can not be null.

Clearly it is being called with null, otherwise your check would not catch it either.

muthu90tech · 2022-12-21T20:49:07Z

This is a very unreadable way of checking pcidl for null, please do that instead.

? Wouldn't he need to check "pcidl->mkid" for being NULL instead? If pcidl was indeed NULL, then his code would not work, but crash due to null-dereference already.

No, checking pcidl->mkid is also wrong if pcidl is null.

The GetInfoFromPidl(pidl, &info) gets called only if pidl in not null. Please see: https://github.com/reactos/reactos/blob/master/dll/shellext/ntobjshex/regfolder.cpp#L191

You have checked just one invocation: https://git.reactos.org/?p=reactos.git&a=search&h=HEAD&st=grep&sr=1&s=GetInfoFromPidl

So pcidl in GetInfoFromPidl can not be null.

Clearly it is being called with null, otherwise your check would not catch it either.

Ok how about checking if pcidl is not null before checking pcidl->mkid in GetInfoFromPidl?

Like:

if (!pcidl)
{
    DbgPrint("PCIDL is NULL\n");
    return E_INVALIDARG;
}

HBelusca

OK; if pcidl is NULL then wouldn't this mean the GetInfoFromPidl function is called "incorrectly"? (if it's not expected to have either of its parameters being NULL)

HBelusca · 2022-12-22T00:30:03Z

dll/shellext/ntobjshex/ntobjfolder.cpp

@@ -484,6 +484,12 @@ HRESULT CNtObjectFolder::GetInfoFromPidl(LPCITEMIDLIST pcidl, const NtPidlEntry
 {
    NtPidlEntry * entry = (NtPidlEntry*) &(pcidl->mkid);



Suggested change

HBelusca · 2022-12-22T00:30:11Z

dll/shellext/ntobjshex/regfolder.cpp

@@ -428,6 +428,12 @@ HRESULT CRegistryFolder::GetInfoFromPidl(LPCITEMIDLIST pcidl, const RegPidlEntry
 {
    RegPidlEntry * entry = (RegPidlEntry*) &(pcidl->mkid);



Suggested change

learn-more · 2022-12-22T11:36:11Z

dll/shellext/ntobjshex/ntobjfolder.cpp

    NtPidlEntry * entry = (NtPidlEntry*) &(pcidl->mkid);
+    if (!entry)


This check is completely useless.

I confirm that this check is not needed now that the preceeding check has been added (compiled and tested, crash is correctly protected). @muthu90tech : I suggest/support to remove this second useless check as suggested by Mark.

learn-more · 2022-12-22T11:36:27Z

dll/shellext/ntobjshex/regfolder.cpp

    RegPidlEntry * entry = (RegPidlEntry*) &(pcidl->mkid);
+    if (!entry)


Same here, this check is completely useless

I confirm that this check is not needed now that the preceeding check has been added (compiled and tested, crash is correctly protected). @muthu90tech : I suggest/support to remove this second useless check as suggested by Mark.

learn-more · 2022-12-28T10:49:43Z

dll/shellext/ntobjshex/ntobjfolder.cpp


+    NtPidlEntry * entry = (NtPidlEntry*) &(pcidl->mkid);


This is still a convoluted way of writing NtPidlEntry * entry = (NtPidlEntry*)pcidl;,
but whatever.

ghost · 2022-12-31T14:34:44Z

3 approved. Ready for merge ?

[SHELL] Return early from GetInfoFromPidl when mkid is null in

f8a4c30

LPCITEMIDLIST, this fixes crash when attempting to drag n drop items from NTObject Namespace and System Registry.

muthu90tech requested a review from learn-more as a code owner December 18, 2022 08:32

katahiromz changed the title ~~[SHELL] Return early from GetInfoFromPidl when mkid is null in LPCITEMIDLIST~~ [SHELL32] Return early from GetInfoFromPidl when mkid is null in LPCITEMIDLIST Dec 18, 2022

katahiromz added the bugfix For bugfix PRs. label Dec 18, 2022

katahiromz added this to New PRs in ReactOS PRs via automation Dec 18, 2022

JoachimHenze approved these changes Dec 18, 2022

View reviewed changes

dll/shellext/ntobjshex/ntobjfolder.cpp Outdated Show resolved Hide resolved

ReactOS PRs automation moved this from New PRs to Approved by reviewers Dec 18, 2022

binarymaster added the shell All PR's related to the shell (and shell extensions) label Dec 18, 2022

binarymaster changed the title ~~[SHELL32] Return early from GetInfoFromPidl when mkid is null in LPCITEMIDLIST~~ [NTOBJSHEX] Return early from GetInfoFromPidl when mkid is null in LPCITEMIDLIST Dec 18, 2022

learn-more requested changes Dec 18, 2022

View reviewed changes

ReactOS PRs automation moved this from Approved by reviewers to WIP / Waiting on contributor Dec 18, 2022

muthu90tech requested a review from learn-more December 21, 2022 07:18

HBelusca approved these changes Dec 22, 2022

View reviewed changes

[NTOBJSHEX] check if pcidl not null

f10a068

learn-more reviewed Dec 22, 2022

View reviewed changes

[NTOBJSHEX] Remove null check which is not needed anymore

ccc291d

muthu90tech requested a review from learn-more December 24, 2022 03:35

learn-more approved these changes Dec 28, 2022

View reviewed changes

ReactOS PRs automation moved this from WIP / Waiting on contributor to Approved by reviewers Dec 28, 2022

HBelusca changed the title ~~[NTOBJSHEX] Return early from GetInfoFromPidl when mkid is null in LPCITEMIDLIST~~ [NTOBJSHEX] Return early from GetInfoFromPidl when pcidl is null in LPCITEMIDLIST Dec 31, 2022

HBelusca merged commit 7fff96e into reactos:master Dec 31, 2022

ReactOS PRs automation moved this from Approved by reviewers to Done Dec 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NTOBJSHEX] Return early from GetInfoFromPidl when pcidl is null in LPCITEMIDLIST #4946

[NTOBJSHEX] Return early from GetInfoFromPidl when pcidl is null in LPCITEMIDLIST #4946

muthu90tech commented Dec 18, 2022 •

edited by HBelusca

ghost commented Dec 18, 2022

learn-more left a comment

JoachimHenze commented Dec 18, 2022

learn-more commented Dec 21, 2022 •

edited

muthu90tech commented Dec 21, 2022

learn-more commented Dec 21, 2022

muthu90tech commented Dec 21, 2022 •

edited

HBelusca left a comment

HBelusca Dec 22, 2022

HBelusca Dec 22, 2022

learn-more Dec 22, 2022

ghost Dec 22, 2022 •

edited by ghost

learn-more Dec 22, 2022

ghost Dec 22, 2022 •

edited by ghost

learn-more Dec 28, 2022

ghost commented Dec 31, 2022

		@@ -484,6 +484,12 @@ HRESULT CNtObjectFolder::GetInfoFromPidl(LPCITEMIDLIST pcidl, const NtPidlEntry
		{
		NtPidlEntry * entry = (NtPidlEntry*) &(pcidl->mkid);

		@@ -428,6 +428,12 @@ HRESULT CRegistryFolder::GetInfoFromPidl(LPCITEMIDLIST pcidl, const RegPidlEntry
		{
		RegPidlEntry * entry = (RegPidlEntry*) &(pcidl->mkid);

		NtPidlEntry * entry = (NtPidlEntry*) &(pcidl->mkid);
		if (!entry)

[NTOBJSHEX] Return early from GetInfoFromPidl when pcidl is null in LPCITEMIDLIST #4946

[NTOBJSHEX] Return early from GetInfoFromPidl when pcidl is null in LPCITEMIDLIST #4946

Conversation

muthu90tech commented Dec 18, 2022 • edited by HBelusca

Purpose

Proposed changes

ghost commented Dec 18, 2022

learn-more left a comment

Choose a reason for hiding this comment

JoachimHenze commented Dec 18, 2022

learn-more commented Dec 21, 2022 • edited

muthu90tech commented Dec 21, 2022

learn-more commented Dec 21, 2022

muthu90tech commented Dec 21, 2022 • edited

HBelusca left a comment

Choose a reason for hiding this comment

HBelusca Dec 22, 2022

Choose a reason for hiding this comment

HBelusca Dec 22, 2022

Choose a reason for hiding this comment

learn-more Dec 22, 2022

Choose a reason for hiding this comment

ghost Dec 22, 2022 • edited by ghost

Choose a reason for hiding this comment

learn-more Dec 22, 2022

Choose a reason for hiding this comment

ghost Dec 22, 2022 • edited by ghost

Choose a reason for hiding this comment

learn-more Dec 28, 2022

Choose a reason for hiding this comment

ghost commented Dec 31, 2022

muthu90tech commented Dec 18, 2022 •

edited by HBelusca

learn-more commented Dec 21, 2022 •

edited

muthu90tech commented Dec 21, 2022 •

edited

ghost Dec 22, 2022 •

edited by ghost

ghost Dec 22, 2022 •

edited by ghost