[Patch] fix IDL hang V2

[Jelmer Vernooij]

On Thu, Aug 21, 2014 at 08:52:11PM +0100, Noel Power wrote:
> On 21/08/14 17:34, Jelmer Vernooij wrote:
> > On Thu, Aug 21, 2014 at 12:01:28PM +0100, Noel Power wrote:
> >> On 07/08/14 12:22, Noel Power wrote:
> >>> Hi,
> >>>
> >>> issuing
> >>>
> >>> ./pidl/pidl --header --ndr-parser -- foo.idl
> >>>
> >>> will cause pidl to hang, the attached patch prevents the recursion that
> >>> triggers that. Note: I compared the contents of
> >>> bin/default/librpc/gen_ndr before and after the patch and the contents
> >>> are identical
> >>>
> >>> Noel
> >> Note previous version had a bug, attached in a newer version, and also
> >> less error-filled example idl(s) ( one that can be used with midl, one
> >> with pidl). As previously mentioned it appears such a recursive
> >> structure arrangement seems to cause MIDL to barf [1], I still don't
> >> believe that is a reason for pidl to similarly fail (or hang), note:
> >> also the ndr marshall/unmarshall generated work fine
> > I'm wary of supporting constructions that are pidl-specific. We'd like to
> > make our IDL files more compatible with midl, not less.
> That's not at all clear from the current idl that seems littered with
> pidl specific extensions. The fact that those specific properties aid
> the the generation of standard marshall/unmarshall routines is imho a
> *good* thing and should be encouraged, hand coding marshall/unmarshall
> routines should be discouraged.
There are a couple of IDL files that have a lot of custom pidl
properties, but for the others it's fairly rare.

Most of the pidl extensions have equivalent attributes in midl. We'd
like to gradually move over from the pidl-specific attributes to the
midl specific ones, so we can more freely exchange IDL files.

pidl extensions also generally don't translate well to wireshark
DCE/RPC calls, making it harder or impossible to generate wireshark dissectors
from IDL files that use pidl extensions.

> > Please add tests for new features (see t/ under the pidl/ directory).
> is it a new feature to prevent a hang? not at all sure how to write a
> test for a hang (note: when it hangs on my machine it quickly makes the
> machine unusable) :-/ but I can try to write a test to see if the
> compile succeeds if that helps...
With your change, presumably it would no longer hang? If the test hangs when
the code is broken, that is not ideal but acceptable.

> not sure what you are getting at with the above, the structure above (if
> converted into idl )
> 
> interface hang
> {
> typedef [public] struct {
> bar a;
> hyper t;
> } foo ;
> 
> typedef [public] struct {
> foo b;
> } bar;
> }
> 
> 
> will hang if compiled with pidl without the patch, with patched pidl it
> will succeed but it will result in generated code that rightly won't
> compile "error: field ‘a’ has incomplete type", however using a pointer
> 
> interface hang
> {
> typedef [public] struct {
> bar *a;
> hyper t;
> } foo ;
> 
> typedef [public] struct {
> foo b;
> } bar;
> }
> 
> and the generated code is compilable, I don't see the problem.
With pointers, why do we need this hack (which is my main concern
with this patch) at all?

If there's a struct member that is a pointer, then
can_contain_deferred should never check whether that member itself can
contain a deferred at all. How does a loop occur here?

FWIW I can compile the second IDL file without problems here, without
your patch:

gwenhwyvar:~/src/samba/pidl% ./pidl --ndr-parser=foo.c --samba3-ndr-client=foo3.c foo.idl
Compiling foo.idl
gwenhwyvar:~/src/samba/pidl%

The first one indeed does cause a hang, but that does seem like a
problematic case which I think we should forbid.

Jelmer

-- 
Jelmer Vernooij <jelmer at samba.org> - https://jelmer.uk/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140822/12f257ed/attachment.pgp>