i#1547 Added secondary tables for 13 syscalls

i#1547 Added secondary tables for 13 syscalls

Added secondary tables for 13 syscalls which are used undocumented enumerations.

NtSetVolumeInformationFile;
NtAlpcQueryInformation
NtAlpcQueryInformationMessage;
NtAlpcSetInformation
NtQueryInformationEnlistment;
NtQueryInformationResourceManager;
NtQueryInformationTransaction;
NtQueryInformationTransactionManager
NtSetInformationEnlistment
NtSetInformationResourceManager
NtSetInformationTransaction
NtSetInformationTransactionManager
NtSetTimerEx

BUG=1547
R=zhaoqin@google.com

Committed: https://code.google.com/p/drmemory/source/detail?r=2109

[Maxim Shudrak, 2014-10-31 20:23:45]

Added secondary tables for 13 syscalls. PTAL

[bruening, 2014-11-04 17:57:05]

Maybe best for load balancing if Qin takes this one too.

On Fri, Oct 31, 2014 at 4:23 PM, <mxmssh@gmail.com> wrote:

> Reviewers: bruening,
>
> Message:
> Added secondary tables for 13 syscalls. PTAL
>
> Description:
> i#1547 Added secondary tables for 13 syscalls
>
> Added secondary tables for 13 syscalls which are used undocumented
> enumerations.
>
> NtSetVolumeInformationFile;
> NtAlpcQueryInformation
> NtAlpcQueryInformationMessage;
> NtAlpcSetInformation
> NtQueryInformationEnlistment;
> NtQueryInformationResourceManager;
> NtQueryInformationTransaction;
> NtQueryInformationTransactionManager
> NtSetInformationEnlistment
> NtSetInformationResourceManager
> NtSetInformationTransaction
> NtSetInformationTransactionManager
> NtSetTimerEx
>
> BUG=1547
> R=bruening@google.com
>
> Please review this at https://codereview.appspot.com/164290045/
>
> Affected files (+4677, -4291 lines):
>   M drsyscall/table_windows_ntoskrnl.c
>   M drsyscall/table_windows_ntoskrnl_infoclass.c
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Dr. Memory Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to drmemory-devs+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[zhaoqin, 2014-11-07 16:35:19]

hmm, did not see that email, will review it now.

On Tue, Nov 4, 2014 at 12:57 PM, 'Derek Bruening' via Dr. Memory Developers
<drmemory-devs@googlegroups.com> wrote:

> Maybe best for load balancing if Qin takes this one too.
>
> On Fri, Oct 31, 2014 at 4:23 PM, <mxmssh@gmail.com> wrote:
>
>> Reviewers: bruening,
>>
>> Message:
>> Added secondary tables for 13 syscalls. PTAL
>>
>> Description:
>> i#1547 Added secondary tables for 13 syscalls
>>
>> Added secondary tables for 13 syscalls which are used undocumented
>> enumerations.
>>
>> NtSetVolumeInformationFile;
>> NtAlpcQueryInformation
>> NtAlpcQueryInformationMessage;
>> NtAlpcSetInformation
>> NtQueryInformationEnlistment;
>> NtQueryInformationResourceManager;
>> NtQueryInformationTransaction;
>> NtQueryInformationTransactionManager
>> NtSetInformationEnlistment
>> NtSetInformationResourceManager
>> NtSetInformationTransaction
>> NtSetInformationTransactionManager
>> NtSetTimerEx
>>
>> BUG=1547
>> R=bruening@google.com
>>
>> Please review this at https://codereview.appspot.com/164290045/
>>
>> Affected files (+4677, -4291 lines):
>>   M drsyscall/table_windows_ntoskrnl.c
>>   M drsyscall/table_windows_ntoskrnl_infoclass.c
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Dr. Memory Developers" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to drmemory-devs+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>>
>
>  --
> You received this message because you are subscribed to the Google Groups
> "Dr. Memory Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to drmemory-devs+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[zhaoqin, 2014-11-10 04:30:04]

LGTM assuming the entries are correct.
Why do we use &sysnum_... in the other review
(https://codereview.appspot.com/168860045/) but not in this review.?

https://codereview.appspot.com/164290045/diff/1/drsyscall/table_windows_ntosk...
File drsyscall/table_windows_ntoskrnl_infoclass.c (left):

https://codereview.appspot.com/164290045/diff/1/drsyscall/table_windows_ntosk...
drsyscall/table_windows_ntoskrnl_infoclass.c:33: 
why remove this line?

https://codereview.appspot.com/164290045/diff/1/drsyscall/table_windows_ntosk...
File drsyscall/table_windows_ntoskrnl_infoclass.c (right):

https://codereview.appspot.com/164290045/diff/1/drsyscall/table_windows_ntosk...
drsyscall/table_windows_ntoskrnl_infoclass.c:775:
ENTRY_SetVolumeInformationFile(NULL, NULL)
in another review, I saw the case like:
ENTRY_QuerySystemInformation(NULL, NULL), &sysnum_QuerySystemInformation
why do not we need &sysnum in this review?

[bruening, 2014-11-10 19:05:20]

On Sun, Nov 9, 2014 at 11:30 PM, <zhaoqin@google.com> wrote:

> LGTM assuming the entries are correct.
> Why do we use &sysnum_... in the other review
> (https://codereview.appspot.com/168860045/) but not in this review.?
>

I didn't look at this review, but I recall the &sysnum discussion from an
earlier review: we want the stored sysnum to match ALL the secondary
numbers, and we want to set it just once, so we had to change the
comparison and move the set to the default entry (secondary table beyond
the max entry).  Look at r2102.

[zhaoqin, 2014-11-11 02:47:38]

On 2014/11/10 19:05:20, bruening wrote:
> On Sun, Nov 9, 2014 at 11:30 PM, <mailto:zhaoqin@google.com> wrote:
> 
> > LGTM assuming the entries are correct.
> > Why do we use &sysnum_... in the other review
> > (https://codereview.appspot.com/168860045/) but not in this review.?
> >
> 
> I didn't look at this review, but I recall the &sysnum discussion from an
> earlier review: we want the stored sysnum to match ALL the secondary
> numbers, and we want to set it just once, so we had to change the
> comparison and move the set to the default entry (secondary table beyond
> the max entry).  Look at r2102.

yes, my question is why we set some sysnum in other CL but not in this one.

[Maxim Shudrak, 2014-11-11 17:46:07]

On 2014/11/11 02:47:38, zhaoqin wrote:
> On 2014/11/10 19:05:20, bruening wrote:
> > On Sun, Nov 9, 2014 at 11:30 PM, <mailto:zhaoqin@google.com> wrote:
> > 
> > > LGTM assuming the entries are correct.
> > > Why do we use &sysnum_... in the other review
> > > (https://codereview.appspot.com/168860045/) but not in this review.?
> > >
> > 
> > I didn't look at this review, but I recall the &sysnum discussion from an
> > earlier review: we want the stored sysnum to match ALL the secondary
> > numbers, and we want to set it just once, so we had to change the
> > comparison and move the set to the default entry (secondary table beyond
> > the max entry).  Look at r2102.
> 
> yes, my question is why we set some sysnum in other CL but not in this one.

We're using &sysnum only for syscalls that need special handling (sysnum is used
as a pointer to handle routine). 
For example we had the following syscall (before r2102):

    {{0,0},"NtQuerySystemInformation", OK|SYSINFO_RET_SMALL_WRITE_LAST, RNTST,
4,
     {
         {0, sizeof(SYSTEM_INFORMATION_CLASS), SYSARG_INLINED,
DRSYS_TYPE_SIGNED_INT},
         {1, -2, W},
         {1, -3, WI},
         {2, sizeof(ULONG), SYSARG_INLINED, DRSYS_TYPE_UNSIGNED_INT},
         {3, sizeof(ULONG), W|HT, DRSYS_TYPE_UNSIGNED_INT},
     }, &sysnum_QuerySystemInformation
    },
Then we use ENTRY_QuerySystemInformation(NULL, NULL),
&sysnum_QuerySystemInformation order to keep it in secondary table.

So the syscalls in this patch don't need special handling therefore we're not
using &sysnums here.

[zhaoqin, 2014-11-11 18:04:49]

yes, that's my guess too, that's why I give LGTM with the question :)

On Tue, Nov 11, 2014 at 12:46 PM, <mxmssh@gmail.com> wrote:

> On 2014/11/11 02:47:38, zhaoqin wrote:
>
>> On 2014/11/10 19:05:20, bruening wrote:
>> > On Sun, Nov 9, 2014 at 11:30 PM, <mailto:zhaoqin@google.com> wrote:
>> >
>> > > LGTM assuming the entries are correct.
>> > > Why do we use &sysnum_... in the other review
>> > > (https://codereview.appspot.com/168860045/) but not in this
>>
> review.?
>
>> > >
>> >
>> > I didn't look at this review, but I recall the &sysnum discussion
>>
> from an
>
>> > earlier review: we want the stored sysnum to match ALL the secondary
>> > numbers, and we want to set it just once, so we had to change the
>> > comparison and move the set to the default entry (secondary table
>>
> beyond
>
>> > the max entry).  Look at r2102.
>>
>
>  yes, my question is why we set some sysnum in other CL but not in this
>>
> one.
>
> We're using &sysnum only for syscalls that need special handling (sysnum
> is used as a pointer to handle routine).
> For example we had the following syscall (before r2102):
>
>     {{0,0},"NtQuerySystemInformation", OK|SYSINFO_RET_SMALL_WRITE_LAST,
> RNTST, 4,
>      {
>          {0, sizeof(SYSTEM_INFORMATION_CLASS), SYSARG_INLINED,
> DRSYS_TYPE_SIGNED_INT},
>          {1, -2, W},
>          {1, -3, WI},
>          {2, sizeof(ULONG), SYSARG_INLINED, DRSYS_TYPE_UNSIGNED_INT},
>          {3, sizeof(ULONG), W|HT, DRSYS_TYPE_UNSIGNED_INT},
>      }, &sysnum_QuerySystemInformation
>     },
> Then we use ENTRY_QuerySystemInformation(NULL, NULL),
> &sysnum_QuerySystemInformation order to keep it in secondary table.
>
> So the syscalls in this patch don't need special handling therefore
> we're not using &sysnums here.
>
>
> https://codereview.appspot.com/164290045/
>

[Maxim Shudrak, 2014-11-14 16:07:25]

Committed patchset #1 manually as 2109 (presubmit successful).

[zhaoqin, 2014-11-25 16:13:41]

a few comment, might be better with another round of review.

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
File drsyscall/drsyscall_windows.c (right):

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
drsyscall/drsyscall_windows.c:483: true/*add syscall in secondary hashtable*/);
so we do not have to check the result of add syscall entry here.
If it is always true/false, add a debug ASSERT would be better.

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
drsyscall/drsyscall_windows.c:494: true/*add syscall in secondary hashtable*/);
ditto

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
drsyscall/drsyscall_windows.c:501: bool ok = false;
typically, we do not init variable here.

detecting an uninit variable read is easier than finding a bug of using wrong
value of a variable.

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
drsyscall/drsyscall_windows.c:600: ok = add_syscall_entry(drcontext, data,
&syscall_ntdll_info[i], NULL, true, false);
line too long.

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
drsyscall/drsyscall_windows.c:617: false/*already added*/, false);
nit: need better alignment

https://codereview.appspot.com/164290045/diff/180001/drsyscall/table_windows_...
File drsyscall/table_windows_ntoskrnl.c (right):

https://codereview.appspot.com/164290045/diff/180001/drsyscall/table_windows_...
drsyscall/table_windows_ntoskrnl.c:126: extern syscall_SetTimerEx_info[];
why do we add a new syscall in this diff?
Is it in part of the 13 syscalls?

[Maxim Shudrak, 2014-12-06 06:15:24]

fixed all comments

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
File drsyscall/drsyscall_windows.c (right):

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
drsyscall/drsyscall_windows.c:483: true/*add syscall in secondary hashtable*/);
On 2014/11/25 16:13:41, zhaoqin wrote:
> so we do not have to check the result of add syscall entry here.
> If it is always true/false, add a debug ASSERT would be better.

Done.

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
drsyscall/drsyscall_windows.c:494: true/*add syscall in secondary hashtable*/);
On 2014/11/25 16:13:40, zhaoqin wrote:
> ditto

Done.

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
drsyscall/drsyscall_windows.c:501: bool ok = false;
On 2014/11/25 16:13:40, zhaoqin wrote:
> typically, we do not init variable here.
> 
> detecting an uninit variable read is easier than finding a bug of using wrong
> value of a variable.

Done.

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
drsyscall/drsyscall_windows.c:600: ok = add_syscall_entry(drcontext, data,
&syscall_ntdll_info[i], NULL, true, false);
On 2014/11/25 16:13:40, zhaoqin wrote:
> line too long.

Done.

https://codereview.appspot.com/164290045/diff/180001/drsyscall/drsyscall_wind...
drsyscall/drsyscall_windows.c:617: false/*already added*/, false);
On 2014/11/25 16:13:40, zhaoqin wrote:
> nit: need better alignment

Done.

https://codereview.appspot.com/164290045/diff/180001/drsyscall/table_windows_...
File drsyscall/table_windows_ntoskrnl.c (right):

https://codereview.appspot.com/164290045/diff/180001/drsyscall/table_windows_...
drsyscall/table_windows_ntoskrnl.c:126: extern syscall_SetTimerEx_info[];
On 2014/11/25 16:13:41, zhaoqin wrote:
> why do we add a new syscall in this diff?
> Is it in part of the 13 syscalls?

Yes this is a part of the 13 syscalls. This syscall was in the previous commit
(which was reverted) but I excluded it b/c it generated an error. Now it works
correct and I can return it.

[zhaoqin, 2014-12-17 15:25:21]

LGTM

By the way, we moved our repository to github, so please commit code there.

[bruening, 2014-12-17 16:05:02]

On 2014/12/17 15:25:21, zhaoqin wrote:
> LGTM
> 
> By the way, we moved our repository to github, so please commit code there.

Please also add the "Review-URL: https://codereview.appspot.com/${number}" line
at the end of the commit message, which "git review" automatically adds.

For migrated diffs from git-svn or svn in the future, please set up for "git
review" (you can append to the same codereview.appspot.com/${number} page by
running "git config branch.${branch}.rietveldissue ${number}").