networks.txt ignored if last line doesn't end in \n

RESOLVED FIXED

Status

Other Applications
ChatZilla
--
minor
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Kenneth Herron, Assigned: Gijs)

Tracking

({polish})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cz-0.9.76])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
The code which reads networks.txt (see bug 27807) silently ignores any text in the file following the last \r or \n. As a result, if the last END tag in the file isn't followed by a newline, the entire file is silently rejected as invalid.

Apparently there's some plan to add network editing to the chatzilla UI, in which case I presume this file would be automatically generated, but currently the only way to get this file is for the user to construct one with a text editor. It's pretty easy to create a file with this flaw in notepad for example. Silently rejecting the file because of a single missing trailing whitespace character is pretty unfriendly behavior.

Based on a conversation in #chatzilla I expect this to be closed without action, but I wanted to get it on record.
(Reporter)

Comment 1

12 years ago
Created attachment 232183 [details]
Sample networks.txt which displays the problem

This defines a network called "nosenet". Note there is no newline following the last "END" tag.
(Assignee)

Comment 2

12 years ago
Created attachment 244423 [details] [diff] [review]
Patch

This seems to work jolly well :-)
Assignee: rginda → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #244423 - Flags: review?(silver)
(Assignee)

Comment 3

12 years ago
Created attachment 244424 [details] [diff] [review]
Patch v2

OK, so perhaps this is the less evil solution.
Attachment #244423 - Attachment is obsolete: true
Attachment #244424 - Flags: review?(silver)
Attachment #244423 - Flags: review?(silver)

Comment 4

12 years ago
Comment on attachment 244424 [details] [diff] [review]
Patch v2

>-            this._buffer += this._fileStream.read();
>+            newBytes = this._fileStream.read();
>+            // If we got something, parse stuff
>+            // Otherwise, bail if there's no data at all.
>+            if (newBytes != null)
>+            {
>+                this._buffer += newBytes;
>+                bytesRead += newBytes.length;
>+            }
>+            else if (bytesRead < this._file.fileSize)
>+            {
>+                continue;
>+            }

This should be rearranged to have the continue first, and perhaps a comment to indicate that we've been told to expect more data but that it hasn't been given to us.

>             // Got more data in the buffer, so split into lines.
>-            // The last one doesn't count - the rest get added to the full list.
>+            // The last one doesn't count if it isn't really the last line.
>             var lines = this._buffer.split(/[\r\n]+/);
>-            this._buffer = lines.pop();
>+            if (bytesRead < this._file.fileSize)
>+                this._buffer = lines.pop();

This will leave this._buffer with the old data if bytesRead >= this._file.fileSize. That doesn't sound right to me.

>             this._lines = this._lines.concat(lines);
Attachment #244424 - Flags: review?(silver) → review-
(Assignee)

Comment 5

12 years ago
Created attachment 244425 [details] [diff] [review]
Patch v3

<Silver_work>	Also, we have another issue I've just realised.
<Silver_work>	You're only counting data read /that call/, when the file is shared over all deserialize calls.
<Hannibal>	ok, so bytesRead should be a member variable?
<Silver_work>	A private one, yeah.

That and comments above fixed, or so I hope.
Attachment #244424 - Attachment is obsolete: true
Attachment #244425 - Flags: review?(silver)

Comment 6

12 years ago
Comment on attachment 244425 [details] [diff] [review]
Patch v3

>+            if (!newBytes && this._bytesRead < this._file.fileSize)

Please use an extra set of parentheses here, for clarity.

r=silver with that fixed.
Attachment #244425 - Flags: review?(silver) → review+
(Assignee)

Comment 7

12 years ago
Created attachment 244833 [details] [diff] [review]
Patch v4

Patch which fixes the nsLocalFile.read method to:
- return null at EOF
- return "" ONLY if the underlying filestream throws while trying to get more data.
- return more data otherwise.

and fixes the users of said method to cope with this change.
Attachment #244425 - Attachment is obsolete: true
Attachment #244833 - Flags: review?(silver)

Comment 8

12 years ago
Comment on attachment 244833 [details] [diff] [review]
Patch v4

>+// Will return null if there is no more data in the file.
>+// Will block as long as it thinks there's data left before |max| .

Not quite right; the stream read method will return > 0 but <= max bytes. It may not return == max always. I think the correct notion here is that it will block until it has /some/ data to return.

>+// Will return an empty string if there is data, but it couldn't be read.


>-            this._buffer += this._fileStream.read();
>-            // Got more data in the buffer, so split into lines.
>-            // The last one doesn't count - the rest get added to the full list.
>+            var newData = this._fileStream.read();
>+            if (newData)
>+                this._buffer += newData;
>+            else if (this._buffer.length == 0)
>+                break;

Nit: Insert a blank line here for clarity.

>+            // Got more data in the buffer, so split into lines. Unless we're
>+            // done, the last one might not be complete yet, so save that one.


>-        state.loadPendingData += state.loadFile.read(state.loadChunk);
>+
>+        var newChunk = state.loadFile.read(state.loadChunk);
>+        if (newChunk != null)
>+            state.loadPendingData += newChunk;

Arguably, that could just be |if (newChunk)| as well.

r=silver with changes.
Attachment #244833 - Flags: review?(silver) → review+
(Assignee)

Comment 9

12 years ago
Fixed with nits changed:

Checking in mozilla/extensions/irc/js/lib/file-utils.js;
/cvsroot/mozilla/extensions/irc/js/lib/file-utils.js,v  <--  file-utils.js
new revision: 1.11; previous revision: 1.10
done
Checking in mozilla/extensions/irc/js/lib/text-serializer.js;
/cvsroot/mozilla/extensions/irc/js/lib/text-serializer.js,v  <--  text-serializer.js
new revision: 1.6; previous revision: 1.5
done
Checking in mozilla/extensions/irc/xul/content/channels.js;
/cvsroot/mozilla/extensions/irc/xul/content/channels.js,v  <--  channels.js
new revision: 1.3; previous revision: 1.2
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Whiteboard: [cz-0.9.76]
You need to log in before you can comment on or make changes to this bug.