Bug 1676 - Uninitialized "out" param for sequence<string> can cause server to core
Summary: Uninitialized "out" param for sequence<string> can cause server to core
Status: RESOLVED FIXED
Alias: None
Product: TAO
Classification: Unclassified
Component: ORB (show other bugs)
Version: 1.5.1
Hardware: All All
: P3 normal
Assignee: vridosh
URL:
Depends on:
Blocks: 1638 1667 1942
  Show dependency tree
 
Reported: 2003-12-16 14:45 CST by Nanbor Wang
Modified: 2006-12-04 04:54 CST (History)
0 users

See Also:


Attachments
Proposed patch for this issue (455 bytes, patch)
2006-11-28 10:03 CST, vridosh
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nanbor Wang 2003-12-16 14:45:07 CST
Okay, if the servant implementation does not initialize the out parameter for sequence of 
<strings>, the server will core trying to marshal it. The ORB needs to be more robust in handling 
this. This is a blocker for 1.4
Comment 1 Nanbor Wang 2003-12-16 14:45:44 CST
Added a blocks comment
Comment 2 Dale Wilson 2003-12-16 15:51:55 CST
Making the servant implemementation more robust should not interpreted as 
making it ignore the uninitialized data.  The application program has provided 
un-marshable data so the correct action in this case is to throw a MARSHAL 
exception.  

Masking the application error will lead to much more serious and hard to 
diagnose system failures.
Comment 3 Nanbor Wang 2003-12-16 16:45:00 CST
Dale, There are two different issues here. If the application makes malformed sequence ie. the 
length being 2000 and the number of strings in the sequence being 0 or something equally bizzare 
then your comment applies. 

If the application doesn't want to "send" anything, it shouldn't need to touch the pointer at all. This 
is not unmarshallable data. It is ORB's responsibility to take care of this. If you don't handle this 
properly, people are going to get fedup with CORB. Throwing a MARSHAL exception is going to 
chase them away. 
Comment 4 Dale Wilson 2003-12-17 09:48:47 CST
Hi Bala,

I consider an uninitialized out parameter or return value an error.  If it 
happened in my code, I would prefer that it result in an error indication 
rather than in undefined behavior.  This is similar to the philosophy of 
treating compiler warnings as errors rather than as noise.

However, I couldn't find a definitive statement about this in the standard, so 
it comes down to a judgement call on the part of the implementor.  Since you 
are more in-tune with the implementation than I am, I won't argue with your 
interpretation of what will sell in the marketplace.

Implementation detail.  To make this work I guess it would be necessary 
to "preinitialize" out parameters before calling the method implementation so 
they would have a safe value if the application failed to assign one, right?

Regards,
Dale


Comment 5 Steve Totten 2003-12-18 09:56:21 CST
The C++ Mapping Specification v. 1.1 (formal/03-06-03), section 1.22,
p. 1-106 states:

<quote>
A callee may not return a null pointer under any of the following
circumstances:

Comment 6 Nanbor Wang 2003-12-18 10:38:56 CST
Oh, gosh! I missed this totally, when I was going over the spec. Anyway, things crash now. Once we 
fix that problem, we can close the bug. Thanks Steve. 


Comment 7 Nanbor Wang 2004-01-03 09:32:23 CST
Accepting this
Comment 8 Johnny Willemsen 2006-04-13 09:21:05 CDT
Bala, do you remember what you your approach would be to fix this bug?
Comment 9 Johnny Willemsen 2006-04-14 04:07:46 CDT
to Kees, he will create a regression for this.
Comment 10 Johnny Willemsen 2006-04-24 05:20:53 CDT
bug still happens with cvs head, regression is added to cvs. problem seems to be
i n Var_Size_SArgument_T.cpp. Code executes is below. Because the sequence is
not explicitly initialized this->x_.in() returns a 0 pointer

template<typename S,
         typename S_var,
         typename Insert_Policy>
CORBA::Boolean
TAO::Out_Var_Size_SArgument_T<S,S_var,Insert_Policy>::marshal (TAO_OutputCDR &cdr)
{
  return cdr << this->x_.in ();
}
Comment 11 Johnny Willemsen 2006-04-25 03:50:55 CDT
we could do something like this in the marshal methods

  if (0 == &_tao_seq) // Trying to de-reference NULL object
    throw BAD_PARAM;
Comment 12 Johnny Willemsen 2006-04-26 03:13:28 CDT
proposal to add

  if (0 == &_tao_sequence) // Trying to de-reference NULL object
      throw ::CORBA::BAD_PARAM(0, CORBA::COMPLETED_MAYBE);
Comment 13 Johnny Willemsen 2006-04-26 04:45:12 CDT
Wed Apr 26 09:44:12 UTC 2006  Kees van Marle  <kvmarle@remedy.nl>

        * tao/Bounded_Sequence_CDR_T.h:
        * tao/Unbounded_Sequence_CDR_T.h:
          Check in all marshal_sequence methods if we aren't trying to
          marshal a nill sequence, this can happen when the user doesn't
          initialize an out argument. In that case we throw a BAD_PARAM
          exception as described in the C++ spec. This fixes bugzilla bug
          1676.
Comment 14 Johnny Willemsen 2006-07-04 02:03:59 CDT
reopening bug, works perfect on windows but seems to fail on linux. so far as I
remember things worked directly after committing the fix but fails again after
some refactoring. no blocker for 1.5.2
Comment 15 vridosh 2006-11-28 10:03:46 CST
Created attachment 632 [details]
Proposed patch for this issue
Comment 16 Johnny Willemsen 2006-11-28 13:01:56 CST
yes, this is better, only NULL is not portable, use 0. We then need to revert
the changes we did earlier to resolve this bug. We have some more checks like
this in generated code, maybe we can move them also
Comment 17 Johnny Willemsen 2006-11-28 13:12:58 CST
reassign
Comment 18 vridosh 2006-11-29 03:11:22 CST
Accepting this
Comment 19 vridosh 2006-11-30 08:49:42 CST
Thu Nov 30 14:48:57 UTC 2006  Vadym Ridosh <vridosh@prismtech.com>
        * tao/PortableServer/Var_Size_SArgument_T.cpp
        * tao/Bounded_Sequence_CDR_T.h
        * tao/Unbounded_Sequence_CDR_T.h

          Fix for bugzilla 1676 (uninitialized "out" param for sequence<string> 
          can cause server to core). Old attempt to fix this was also removed.

Committed revision 75713.
Comment 20 vridosh 2006-11-30 08:50:33 CST
Xref: TAO#449
Comment 21 vridosh 2006-11-30 08:52:48 CST
Xref: TAO#419
Previous Xref (449) was invalid.
Comment 22 Johnny Willemsen 2006-12-04 04:54:05 CST
fixed, test stats also show this