Re: [boost] [afio] AFIO review postponed till Monday

Subject: Re: [boost] [afio] AFIO review postponed till Monday
From: Michael Marcin (mike.marcin_at_[hidden])
Date: 2015-07-23 22:10:01

On 7/22/2015 8:11 PM, Niall Douglas wrote:
> On 22 Jul 2015 at 13:35, Glen Fernandes wrote:
>
>> 1. Use of documented kernel APIs: I'm torn here; on one hand, I don't
>> condemn use of them and I would agree with Asbjørn that it isn't a show
>> stopper. Yet you have a case of an actual show-stopper (crash) in AFIO
>> because of the use of them.[1]
>
> The show stopper crash is due to how ASIO implements its IOCP reactor
> which just happens to collide with the WOW64 bug. If I could modify
> the ASIO source code to special case this it's fixed. This is why
> Microsoft marked it as wonfix because even though it's a bug in their
> code, it is really super easy to work around if you modify your IOCP
> reactor.
>
> As I mentioned before, if I don't hand off the async to ASIO I make
> the problem go away.
>
>> 2. Use of undocumented or deprecated APIs: Kernel APIs or otherwise, even if
>> apparently innocuous, this practice is alarming to me. I would be surprised
>> if it doesn't taint the review of AFIO.
>
> I can replace ObjectNameInformation with a new routine which uses
> non-deprecated APIs if the review demands it. Indeed, the
> ObjectNameInformation use replaced some original code which iterated
> every mounted volume in the system per path lookup, which was costly.
>
> I suppose one _could_ make the assumption that open file handles can
> never be relocated across mounted volumes and that mounted volumes
> are never remounted anywhere new while file handles are open within
> them. With those two assumptions I can use NtQueryFileInformation
> instead, and reconstruct the same thing NtQueryObject returns.
>
> What worried me was the possibility that a volume could be mounted in
> two or more locations you see, and that the canonical path of a file
> could change, hence the iteration of all mounted volumes to check for
> that.
>
>>> I think this is the wrong question to ask.  The right question to ask is:
>>> [snip]
>>
>> So the wrong question to ask is, "Is it concerning that AFIO uses these APIs
>> given your use of them is unsupported?" and the right question to ask is "Is
>> it concerning that AFIO use these APIs given your use of them is unsupported
>> but the advantages are [...]?"  ?  :-)
>
> There is unsupported and there is unsupported. If you want to write a
> user mode program which executes in the Windows boot sequence before
> the subsystems start up (i.e. there is no win32 API available yet),
> then you write that program using the NT kernel API. That's
> supported, because it's categorised as a DDK user mode application.
>
> By unsupported Microsoft have never said "don't use these APIs
> directly", they actually said "we won't promise we won't change
> anything inside the header file Winternl.h" and it just happens that
> the kernel API falls into that header file along with a ton of other
> stuff. I may have mentioned earlier that Cygwin and Mingw use the
> same APIs as AFIO, and if you review the Windows bug tracker where
> those projects report bugs in Windows you'll find a common pattern of
> the first tier support in Microsoft refusing the fix the bug as its
> use is "unsupported", then a more senior tier fixes the bug anyway.
>
> That's because the single biggest user of the NT kernel API is
> Microsoft itself, and Microsoft would internally like bugs in one of
> its most fundamental APIs fixed, so they take these bug reports
> seriously.
>
> If this isn't "supported", it looks awfully like supported. I would
> actually say the real situation is the Microsoft doesn't _encourage_
> direct user mode use of NT kernel APIs. Historically speaking it
> wouldn't have worked on WinCE and Win95, and that was once a problem.
> I think we're a long way away from that now.
>
> BTW the Visual Studio MSVCRT makes extensive use of NT kernel calls
> itself, indeed Stephan pointed me at RtlGenRandom which solved a
> problem for me.
>
>> Niall Douglas wrote:
>>> I would say all three of these yes.
>>
>> Sounds like a good enough spot to put yourself out of your misery and take
>> this opportunity to escape the review queue that you've been in for so long.
>> I understand wanting to improve the library, but something tells me you
>> don't want to be in the queue for another two years constantly refactoring
>> AFIO. Just my two cents.
>
> I would certainly prefer review sooner rather than later. It could be
> if AFIO had been reviewed much earlier on I wouldn't need to do so
> much refactoring now, but maybe it's too niche for that. I don't
> know, I haven't even proved the case for futures instead of
> async_result yet, and I already know the ASIO crowd here is going to
> be united on that.
>

FWIW as a user I fully expect Boost to internally use undocumented APIs 
or even potentially unsafe code if necessary to make things 
smaller/faster/better, and handle all of the edge cases that make them 
dangerous and hide it all behind a nice interface.

Basically exactly what AFIO is doing here.

If what's in Boost isn't optimal then I'm usually going to have to 
consider rolling my own or using some other library which will be less 
tested, less documented, and less peer-reviewed.