BufferedWriter doesn't send text unless stream is closed

Question

I have a server that communicates with a client.

The server is multithreaded, and this thread is created with the socket and a bufferedreader connected to the socket, when the first line read from the socket is "request":

public class scriptComm implements Runnable {

private Socket sock;
private Socket sock2;
private Connection connection;
private BufferedReader reader;

@Override
public void run() {
    try {

        String name = reader.readLine();
        String password = reader.readLine();

        String line;
        connection = methods.connectToDatabase();
        BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(sock.getOutputStream()));
        if (connection != null && name != null && password != null) {
            try {
                ResultSet rs = connection.createStatement().executeQuery(
                        "SELECT name, password, doupdate FROM accounts "
                        + "WHERE name = '" + name + "' AND doupdate = 'yes'"
                        + " AND password = '" + password + "'");
                if (rs.next()) {
                    methods.log("worked");
                    bw.write("accept");
                    bw.flush();
                    bw.close();
                    reader = new BufferedReader(new InputStreamReader(sock2.getInputStream()));
                    if ((line = reader.readLine()) != null) {
                        mainFrame.jTextArea1.append("line \n");
                        connection.createStatement().executeUpdate(
                                "UPDATE accounts SET updatetext = '" + line + "' "
                                + "WHERE name = '" + name + "'");
                    }else{
                        mainFrame.jTextArea1.append("No text received \n");
                    }
                } else {
                    bw.write("decline");
                    bw.flush();
                }
                bw.close();
                rs.close();
            } catch (SQLException ex) {
                methods.log("Error when executing statement in scriptComm");
                ex.printStackTrace();
            } catch (IOException ex) {
                ex.printStackTrace();
            }
        } else {
            methods.log("missing values in scriptComm");

        }
    } catch (IOException ex) {
    }


}

public scriptComm(Socket sock, BufferedReader reader) {
    this.sock = sock;
    this.sock2 = sock;
    this.reader = reader;
}}

You may notice that I close the bw stream after it writes "accept".

This is due to the client simply hanging there, as if it doesn't receive any input, when the stream is not closed.

The client:

        try{
        String line;
        Socket sock = new Socket("myipaddresshere",portnumber); 
        PrintWriter writer = new PrintWriter(sock.getOutputStream());
        BufferedReader reader = new BufferedReader(new InputStreamReader(sock.getInputStream()));
        writer.println("script");
        writer.flush();
        writer.println(jTextField1.getText());
        writer.flush();
        writer.println(jTextField2.getText());
        writer.flush();
        if ((reader.readLine()).equals("accept")) {
            writer.write("testing123");
            writer.flush();
            writer.close();
        } else {
            jTextArea1.append("fail");

        }
        reader.close();
        writer.close();
    }catch(IOException e){
        jTextArea1.append("Server not available. Please try again later.");
    }

When the stream is not closed after writing "accept" from the server, it's as if the client just sits at the if(reader.readLine().equals("accept")) boolean check ( and yes, the stream is flushed server-side ).

However, when the stream is also closed server-side, it passes the boolean check and continues on to write the line "testing123" to the stream. The server can obviously not read this line as the socket stream was closed when the BufferedReader closed. As you may notice, I tried replicating the socket by simply creating another variable called sock2, but it seems this connection closes as well (makes sense).

Note: when connecting with a wrong user/pass (i.e. when rs.next() returns false), it does write "decline" to the stream and the client gets this.

Really confused about this one..

Thanks, Mike.

score 3 · Accepted Answer · answered Jul 25 '11 at 10:55

3

Note, a write doesn't write a newline, and you are trying to read whole lines. You need to write a newline character before you flush.

Edither use BufferedWriter.newLine() or append "\n" to the strings that you write.

answered Jul 25 '11 at 10:55

Kaj

10,862
2
33
27

Seems so silly that I didn't spot that :p. thanks. Also: any tips on where to start reading about preventing sql-injections? – Mike Haye Jul 25 '11 at 12:17
1

Google on it, but what you should do is to switch to a `PreparedStatement` with parameters. That java class will handle escaping for you, so a user name that contains e.g. double or single quote would then no longer be a problem. – Kaj Jul 25 '11 at 12:29
This article explains how to prevent it: https://www.owasp.org/index.php/Preventing_SQL_Injection_in_Java – Kaj Jul 25 '11 at 12:30
Using PreparedStatement now. Also, the strings aren't allowed to be longer than 20 characters, and must not include * or = or the phrase select. That should provide moderate protection, right? – Mike Haye Jul 25 '11 at 14:53
The PreparedStatement handles *all* escaping, so you can now also allow usernames that contains *, or anything that looks like sql. E.g a username of 'select * from blah;drop table foo' won't be a problem. – Kaj Jul 25 '11 at 19:25
This says otherwise: https://www.owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet#Defense_Option_1:_Prepared_Statements_.28Parameterized_Queries.29 Says that input validation should be performed – Mike Haye Jul 25 '11 at 20:14
You are reading it wrong. You have 3 options, select one of them. PreparedStatements are totally safe if you use parameterized queries. Option 3, "Escaping All User Supplied Input", should be avoided if you can. Regarding white listing that they write about, it's not needed fore security concerns but you can apply input validation because of business rules (e.g. zip code must look like xxxx) – Kaj Jul 26 '11 at 06:32
Makes sense, but what about this part they write? `String custname = request.getParameter("customerName"); // This should REALLY be validated too // perform input validation to detect attacks String query = "SELECT account_balance FROM user_data WHERE user_name = ? "; PreparedStatement pstmt = connection.prepareStatement( query ); pstmt.setString( 1, custname); ResultSet results = pstmt.executeQuery( ); ` – Mike Haye Jul 26 '11 at 09:53
As I said, it isn't needed for security reasons, but you can validate it for business reasons. You can really test it instead of arguing. Come back when you have been able to perform an sql-injection attack with a parameterized prepared statement. The only thing that you will be able to do is to insert a name that looks like sql. – Kaj Jul 26 '11 at 10:47

parsifal · Answer 2 · 2011-07-25T11:04:09.357

2

I'll leave this retracted with a piece of advice:

When you want to write characters to a stream, you should always use an OutputStreamWriter and InputStreamReader with an explicit character encoding. I recommend "UTF-8".

As written, your code will both write and read with the platform-default encoding. Which will work, right up until the time you have someone in China running the client on Windows, with someone in the US running the server on Linux (and depending on what you're sending, it could break much sooner).

edited Jul 25 '11 at 11:04

answered Jul 25 '11 at 10:56

parsifal

1,507
8
7

That isn't the problem in his case, the problem is that he's using `readLine()` but the server isn't writing newlines. – Kaj Jul 25 '11 at 11:01
@Kaj - yea, I answered based on the title, then saw all the flushes in the code. – parsifal Jul 25 '11 at 11:05
I can give him another piece of advice. Change the database code, it's open to sql injection attacks :) – Kaj Jul 25 '11 at 11:17
Piece of great advice, thanks. Also gonna read up on how to prevent sql-injections. Any tips on where to start reading? – Mike Haye Jul 25 '11 at 12:16

BufferedWriter doesn't send text unless stream is closed

2 Answers2