DERBY-1434 - Client can send incorrect database name to server after having made multiple connections to different databases.

[Julius Stroffek <Ju...@Sun.COM>]

Hi All,

I spend some time analyzing the communication protocol to find out 
something related to solution of DERBY-1434. Because I'm not familiar 
with the Derby communication protocol yet, I would like to get some advice.

When client creates multiple connections to different databases (through 
the JDBC driver), the DRDA protocol 'always' (I'm not sure if always but 
i have not found any situation when it does not) sends the same 
PKGNAMCSN block as the one used for a first created connection.

The reason of this behaviour is in a creation of the Section object in 
SectionManager using it's method 'getDynamicSection'. It creates the 
section by calling the

getSection(freeSections[Non]Hold_, packageNameWith[No]Hold__, 
cursorNamePrefixWith[No]Hold__, resultSetHoldability)

which later calls the init method of the Section class with the 
parameter isGenerated = false. Due to this the buffer which is used to 
store the PKGNAMCSN is initialized to the value stored in 
Agent.SectionManager.holdPKGNAMCBytes - this field is declared static so 
the value is shared among all created connections. The reusing of the 
value from the buffer is probably only a performance improvement and if 
the value in a Section object (created with isGenerated = false) is not 
initialized it is generated on demand and also stored to 
SectionManager.holdPKGNAMCBytes.

I would suggest removing the static modifier from the fields 
noHoldPKGNAMCBytes and holdPKGNAMCBytes of the 
org.apache.derby.client.am.SectionManager class. Each Connection objects 
holds its own Agent object which holds its own SectionManager object. 
Thus, the PKGNAMCSN blocks will be generated once for each Connection 
object. Is this right?

However, the description of DRDA protocol looks like a PKGNAMCSN block 
is important to identify the correct server's sql executional package 
(I'm not an expert, it's only my opinion). How it is possible that 
sending a wrong PKGNAMCSN does not affect the database behavior? In the 
demonstration program on JIRA both connections connect to the same 
server instance. Might connecting these connections to different 
machines (or only server instances) lead to improper behavior? Why 
requests with the wrong PKGNAMCSN still work? I'll try two different 
machines tomorrow (approx. in 16 hours) and let you know.

Regards

Julius Stroffek

Re: DERBY-1434 - Client can send incorrect database name to server after having made multiple connections to different databases.

[Bryan Pendleton <bp...@amberpoint.com>]

> So reading your comments again I think the best way how to implement a 
> test is to add a check to DRDAConnThread object and throw an exception
> when the server receives a different RDBNAM in PKGNAMCSN codepoint than 
> the default values of a connection. The test program then could be
> the program specified in JIRA - when the client sends different package 
> name, the exception will be thrown ("RDBNAM Mismatch") and query
> won't be processed. It is sufficient to add this check only to 
> parsePKGNAMCSN function.

This sounds like an excellent solution to me. The server-side check
seems reasonable and appropriate, and the test sounds like it will
be short, sweet, and to the point. Very good!

One thing we should be very careful about, is whether this new server-side
check introduces a compatibility problem.

We don't want the new server to refuse to work with older clients, because
they still have the bug of sending the wrong database name in certain
circumstances. Since we know that using the wrong database name on the
server side is harmless, we should be forgiving with these old clients
and allow them to work without requiring that they be patched for this issue.

So we should code the server-side check such that it only applies to a 10.3
client or above.

I think that with a bit of hunting, you can find some similar version-compat
checks on the server side that we can mimic for this new check.

And then you'll want to run your test program in two configurations:
1) Your new repro test in 10.3 should trip the check, and get the expected failure
2) The original repro program, if built and run using the 10.2 or 10.1 clients,
should quietly not trip your new check, but of course it will run with the wrong
database name so the traces, etc. will be wrong.

thanks,

bryan

Re: DERBY-1434 - Client can send incorrect database name to server after having made multiple connections to different databases.

[Julius Stroffek <Ju...@Sun.COM>]

Hi Bryan,

I'm sorry for missing your comments in creation of a patch - I came back 
to this issue after a couple of days after reading your comments. Thank 
you for reminding.

So reading your comments again I think the best way how to implement a 
test is to add a check to DRDAConnThread object and throw an exception
when the server receives a different RDBNAM in PKGNAMCSN codepoint than 
the default values of a connection. The test program then could be
the program specified in JIRA - when the client sends different package 
name, the exception will be thrown ("RDBNAM Mismatch") and query
won't be processed. It is sufficient to add this check only to 
parsePKGNAMCSN function.

A small extract from DRDA DDM:
---------------------------------------------
/With SQLAM level 7, the PKGNAMCSN is changed to an optional instance 
variable. If specified,
the values are used as the default package name and consistency token 
for the connection. If not
specified on the command, the PKGSN is required to identify the section 
number. In this case,
the default package name, consistency token, and specified section 
number are used for the
request. If PKGNAMCSN was never specified on the connection, a PRCCNVRM 
is returned.

/According to this specification I think it is possible to change the 
default database name on a connection,
thus theoretically there might exist a request that would require to 
change a database name through
the PKGNAMCSN codepoint. Thus the question is: Will we add a check or 
will we allow a change of
the database name on a connection? It makes no sense to allow a database 
name change for jdbc api...
because AFAIK this stuff is not supported by the jdbc api.

However, I would not recommend allowing a change of the connection just 
now because connecting
with an old implementation of jdbc client to a new implementation of 
server will not work when creating
multiple connections to different databases.

So, I will add these things to the patch:

1.) A check of the database name in PKGNAMCSN codepoint.
2.) A test program doing something similar as the d1434.java attached to 
jira.

Would it be alright then?
> And thanks for the detailed analysis of the problem, Julius; it has been
> quite illuminating to read your notes.
Thanks... ;-); I liked going through it...

Cheers,

Julo

Re: DERBY-1434 - Client can send incorrect database name to server after having made multiple connections to different databases.

[Bryan Pendleton <bp...@amberpoint.com>]

> If #2 is all it takes to fix the trace, and if there are no negative 
> effects, then that's what I vote for. 

I agree. Fixing the trace is important, sending the right database name
in the PKGNAMCSN is important, and solution #2 gets my vote as well.

I think it was important to explore the issue to this level of detail,
to satisfy ourselves that there were no other functional problems besides
the trace (that is, that we didn't execute the wrong queries, or mix up
the results from two queries issued at the same time, or something like that).

Also, I was hoping that we would be able to construct a regression test
for this change, as something other than:
  - run this test program with these trace flags set
  - read the trace files and make sure they look ok
as I would have preferred to see a more automatic regression test of some sort.

But I haven't thought of a simple way to write such a regression test.

Julius, I think that it's still worth incorporating your test program(s) into
the patch as you construct it, even though using those test programs requires
some manual actions (enabling the trace flags, reading the trace files). I think
it would be good to include the instructions for how to use the test program(s)
to verify the correct operation of the patch as code comments in the test programs
themselves. This will be useful at least during the review of the patch, but
I think that there's no harm in including the test programs as a permanent part
of the regression suite, as they exercise a certain path through the code and
shouldn't cause too much of a performance impact.

And thanks for the detailed analysis of the problem, Julius; it has been
quite illuminating to read your notes.

thanks,

bryan

Re: DERBY-1434 - Client can send incorrect database name to server after having made multiple connections to different databases.

[Army <qo...@gmail.com>]

Julius Stroffek wrote:
> Query processing is always performed correctly. The only one visible 
> impact is the server trace.

To an end-user that might not mean much, but to developers trying to read a 
trace in order to track down a problem, I think it's important to have correct 
server traces as much as possible.  At the very least, it makes it so that 
people don't stare at the database name in the trace and think, "Hmm...that 
looks wrong...I wonder if that's the cause of the problem I'm 
investigating?"--which is exactly what I spent a half-day doing before I came to 
the conclusion that it wasn't related :)

> The PKGNAMCSN is not a mandatory code point according to DRDA DDM. For 
> now I see these alternatives:
> 
> 1.) Leave the code unchanged. Everything will work fine except the 
> server traces will be confusing.
> 
> 2.) Remove the static modifier from the fields noHoldPKGNAMCBytes and 
> holdPKGNAMCBytes of the org.apache.derby.client.am.SectionManager class.
> 
> 3.) Also remove the static modifier as described above, but also do not 
> send PKGNAMCSN when it is not needed and send PKGSN instead. This needs 
> some more changes to code. The function buildPKGNAMCSN of the 
> NetPackageRequest uses function canCommandUseDefaultPKGNAMCSN() to check 
> whether it should send PKGNAMCSN or PKGSN. However this function always 
> returns false. It seems that its not fully implemented.

If #2 is all it takes to fix the trace, and if there are no negative effects 
(esp. would removing the static fields have a performance impact?  Would it be 
enough to cause concern?), then that's what I vote for.  #1 seems like a shame 
since, as I mentioned above, things as they are can lead to confusion and 
potentially wasted time.  #3 seems like a slight overkill if #2 solves the 
problem and there's no other impact on query processing...

Army

Re: DERBY-1434 - Client can send incorrect database name to server after having made multiple connections to different databases.

[Julius Stroffek <Ju...@Sun.COM>]

Hi Bryan,

Thank You for your answers. I spent more time on this (see updates in JIRA)

Bryan Pendleton wrote:
> I think this is a key question, and we should figure it out before we
> have a complete change. My hunch is that the reason this doesn't cause
> many symptoms is because the PKGNAMCSN is basically used as a hash
> key on the server side, and so it doesn't really care about the contents
> too much; it is just using those tokens as unique identifiers to
> distinguish the various different statements that the client and server
> are currently working with.
Exactly, PKGNAMCSN is used only for hashing of statements on server side.
The section number in PKGNAMCSN code point is specified correctly 
(that's why using multiple prepared statements on the same connection 
works fine).
> Do you have a test program which demonstrates a concrete, user-visible
> symptom of this problem? The repro program attached to the bug
> causes Derby to write some strange looking trace files, but the actual
> query processing is all performed correctly.
Query processing is always performed correctly. The only one visible 
impact is the server trace.
> So it would be really good if you could construct a test program
> which demonstrates the symptoms of the incorrect PKGNAMCSN field
> more vividly: incorrect query execution, crash or error message on the
> server side, etc.
This is probably not possible.
---

The PKGNAMCSN is not a mandatory code point according to DRDA DDM. For 
now I see these alternatives:

1.) Leave the code unchanged. Everything will work fine except the 
server traces will be confusing.

2.) Remove the static modifier from the fields noHoldPKGNAMCBytes and 
holdPKGNAMCBytes of the org.apache.derby.client.am.SectionManager class.

3.) Also remove the static modifier as described above, but also do not 
send PKGNAMCSN when it is not needed and send PKGSN instead. This needs 
some more changes to code. The function buildPKGNAMCSN of the 
NetPackageRequest uses function canCommandUseDefaultPKGNAMCSN() to check 
whether it should send PKGNAMCSN or PKGSN. However this function always 
returns false. It seems that its not fully implemented.

What do you think?

Cheers

Julo

Re: DERBY-1434 - Client can send incorrect database name to server after having made multiple connections to different databases.

[Bryan Pendleton <bp...@amberpoint.com>]

> I spend some time analyzing the communication protocol to find out 
> something related to solution of DERBY-1434. 

Thanks for looking at this problem!

> I would suggest removing the static modifier from the fields 
> noHoldPKGNAMCBytes and holdPKGNAMCBytes

I think that is a good direction to pursue. I see from the comments in
Jira that Kathey was thinking along the same lines.

> How it is possible that 
> sending a wrong PKGNAMCSN does not affect the database behavior?

I think this is a key question, and we should figure it out before we
have a complete change. My hunch is that the reason this doesn't cause
many symptoms is because the PKGNAMCSN is basically used as a hash
key on the server side, and so it doesn't really care about the contents
too much; it is just using those tokens as unique identifiers to
distinguish the various different statements that the client and server
are currently working with.

If you look at the handling of OPNQRY on the server side (see parseOPNQRY
in DRDAConnThread.java), you'll find that the server is getting the
database name from the RDBNAM field, not from the PKGNAMCSN field, so
that may be a clue to why the server doesn't experience any problems with the
database name being incorrect in the PKGNAMCSN field.

CNTQRY is handled similarly; it passes a separate RDBNAM codepoint.

Do you have a test program which demonstrates a concrete, user-visible
symptom of this problem? The repro program attached to the bug
causes Derby to write some strange looking trace files, but the actual
query processing is all performed correctly.

There is a second repro program in the bug comments, but I think we've
decided that that program had a bug in it, and does not demonstrate
a bug in Derby itself.

So it would be really good if you could construct a test program
which demonstrates the symptoms of the incorrect PKGNAMCSN field
more vividly: incorrect query execution, crash or error message on the
server side, etc.

I think this will make it easier to evaluate the impact of the change
that you propose to the client-side SectionManager.

thanks,

bryan