3.4.x issues

Openfire Plugins Packet Filter
1

I haven’t started testing the Packet Filter with 3.4.x yet. I plan to do it soon. Just wanted to create this thread for anyone to report issues that they are having so I can be sure to double check them in my testing.

Cheers,

Nate

2

Just to keep everything here in this thread, using OpenFire 3.4.1, I am unable to reject/deny packets from group to group. We have a group named Operations. What we want to do is block people that are in the Operations group from chatting with other people in Operations (except for a select few, which we will create allow rules for following the documentation example). I enabled logging to see if anything was being logged about it, but there was nothing. I was able to reject packets between two specific users, just not using groups. Thanks.

Brian

3

Brian,

I’m currently wrapping up work for PacketFilter 2.0. I’ve tested your issue specifically and it works for me. Would you be interested in testing the new version. Let me know and I can get you a copy.

Cheers,

Nate

4

I would definitely be interested in testing it out! We were just a week or two awway from temporarily pulling the openfire server from a group of people due to this. Thank you.

Brian

5

Have you had a chance to try Packet Filter 2.0? If so I would be interested to hear your results.

6

PacketFilter 2.0

Openfire 3.4.4 and Openfire 3.4.5

MySQL 5.0.22

If get this exception with empty rule table…because after fresh install the rule table is normally empty!!

2008.03.14 17:58:23 [org.jivesoftware.openfire.plugin.rules.DbRuleManager.getLastOrderId(DbRuleManager.java:171)]
java.sql.SQLException: Illegal operation on empty result set.

It took a look into DbRuleManager.getLastOrderId() and found this:

con = DbConnectionManager.getConnection();
pstmt = con.prepareStatement(GET_LAST_ORDERID);
rs = pstmt.executeQuery();
rs.next();
count = rs.getInt(1);

That can’t work! Try something like this:

con = DbConnectionManager.getConnection();
pstmt = con.prepareStatement(GET_LAST_ORDERID);
rs = pstmt.executeQuery();
if (rs.next()) {     count = rs.getInt(1);
}
else {
    count = 0;
}
7

Coolcat,

Thanks for reporting this. I’ll dig into it more this weekend. The curious thing is how this was working for me even with an empty rule set. weird.

-Nate

8

There are more bugs…

I can’t remove the plugin. I will have to stop the server and remove the plugin manually. (I planned a complete shutdown on Monday for a kernel update, I will do it then)

I got once this error message in warn.log, but I can’t reproduce it:

2008.03.14 23:09:31 Error unloading plugin packetfilter. Will attempt again momentarily.

I can’t modify rules. If I try to save the modified rule, I get the error “Please specify a valid destination JID or Domain”.

Sorry that I have to ask, but have you really tried this plugin yourself before publishing it? You should mark it as “Beta” or so…then I would never come up the idea to install this plugin on my primary server. I did a quick test on my secondary server, found the first bug, but this was only an error message, not more, so I installed it on the primary… Especially the fact that you can’t remove it if it doesn’t work is really critical!

Also I have to say that a dropdown box for users it not really helpful if you have more than thousand users. I recommend a field “local user” which simply adds the xmpp domain name to the username entered.

9

Performance Tips

A PacketInterceptor processes ALL packets that go through the server, so performance is important here.

1.

A PacketInterceptor processes all packets twice. The first time the flag +processed+ is +false+, second time its +true+. Only in the first case you can drop packets. You should add the following line to +PacketFilterPlugin.interceptPacket()+:

if (processed) { return; }

performance gain: near 100%

2.

Your are using some Enums, ok. But why you convert them to string and compare with equals?! Enums are Singletons, you can safely compare the references. A String in Java has two bytes per character, internally you need some kind of loop to compare two strings. References are nothing else than Integers (4 bytes), which can compared directly by hardware!

3.

Why are you converting JIDs to Strings and parsing them again?

E.g. PacketFilterUtil.getComponent() could easily replaced with JID.getDomain();

4.

Avoid unnecessary method calls…e.g PacketFilterUtil.getDomain() does nothing else than calling getComponent(). Method calls are time consuming because you must create new Stackframe and store all local variables in it. Allocating memory is always expensive.

Class PacketFilter could implement the PacketInterceptor interface directly.

5.

Method PacketFilterUtil.isLocalUser(). This method is…puh! Thats what you are doing there:

  1. Loading all usernames from database and sort them by name. Slow if the database is remote! Sorting takes O(n * log n), retrieving takes O(n).

  2. Comparing the Collection of user in linear time (O(n)) …in average case you have to compare 50% of all usernames!

  3. Each time you compare the username building the string username+"@"+serverName. It would be easier to try username.equals(jid.getNode())…

  4. Why you don’t test if the domain matches the local domain before comparing all usernames?

Better use this one here:

public static boolean isLocalUser(JID jid) {
     return xmppServer.isLocal(jid) &&
            userManager.isRegisteredUser(jid.getNode());
}

This method uses the UserCache which uses internally a HashMap. A HashMap supports searching in O(1), which means it doesn’t matter how many users you have, it is always extremely fast. (Better would be a TreeMap here, a bit slower (O(log n)), but takes less memory.)

If the user was not found in cache, its retrieved from database, but not all users, only the single one you want (O(1))! After retrieving the user is now cached, so next time is much faster.

I assume you have never heard something of theory of computation, theory of complexity, O-notation and such things.

The variable n means always the amount of data, in this case the count of users. O(n) means you need n steps of a constant time to do that want. E.g. if n = 1,000,000,000 and each step takes 1 second, you will take 31 years to complete your mission. But if you have a better algorithm, which takes only O(log n) time, you can do the same work in 30 seconds. Best algorithm would be O(1), which take always this constant time.

You see, you can write a lot better programs, if you have a bit knowledge of complexity.

(This is only a simplified explanation of complexity, check wikipedia, google, …)

After writing this I saw that PacketFilterUtil.isLocalUser() is never used…

10

Found the reason for this:

I can’t remove the plugin.

The *.jar file is named packetFilter.jar, but, I’am not sure why, Openfire deploys a directory packetfilter. On operating systems, such as Linux/Unix, where filenames are case sensitive this is a problem. However, if you can’t remove it, rename packetFilter.jar to packetfilter.jar

11

See bug_report here:

12

Coolcat,

I found the reason that this was working for me. I was doing "

try {
            con = DbConnectionManager.getConnection();
            pstmt = con.prepareStatement(GET_LAST_ORDERID);
            rs = pstmt.executeQuery();
            rs.next();
            count = rs.getInt(1);         } catch (SQLException sqle) {
            Log.error(sqle);
//If error dataset is probably empty
            return 0;
        }

Granted this isn’t a very good way to do things. It seems that MySQL swallows exceptions sometimes so your solution is better. I’ve changed this and will check the code in tomorrow.

13

Coolcat,

Thanks for the code review I must admit that the coding of this was done in a vacuum so having someone else look at the code has been very helpful. Going over your suggestions :

  1. I wasn’t aware of this. I’ve added the processed check to the code.

  2. Fixed in all the Packet Filter code. The jsps are still using the old way but I figure it’s not as important to fix now.

  3. Fixed.

  4. I cleaned this up. The util class only contains one method now. I’m not sure I agree with implementing the PacketInterceptor directly in the PacketFilter class. Separating the packet handling and the rule matching seems cleaner to me.

  5. I did take Data Structures when I was in school I did find one place where I was doing some unnecessary looping. The group rule matching used to be :

for (JID jid : group.getMembers()) {
                if (jid.toBareJID().equals(packetToFrom)) {
                    return true;
                }
            }

Now it does :

if (group.isUser(packetToFrom)) {
                return true;
            }

There are some other inefficient areas but I think in order to fix them would require some serious effort to fix. For example the rule looping could be much more efficient so that instead of comparing a packet to every rule, only the possible matches are compared.

Thanks again for your suggestions. I’ve attached a new version and source if you want to give them a try. I’ll be committing the changes tomorrow.

-Nate
packetFilter.jar (55157 Bytes)
packetFilter.tar.gz (85940 Bytes)

14

If I find time I will try it tomorrow on our testing server.

15

I forgot to ask about the issue with edit rules. Can you still not edit anything?

-Nate

16

I gave it a short test. Because I am running Openfire 3.4.5, I needed make a few changes and recompile the plugin.

Works great now

I get sometimes some of these:

2008.03.17 13:54:27 An answer to a previously sent IQ stanza was never received. Packet id: 31-25

I’m not sure where this comes from.