liborigin-devel

[Liborigin-devel] Bug fixes

[Miquel G. <gbm...@gm...>]

Function Origin800Parser::findStringPos when the passed string
is not found leaves the 'get pointer' of file close to (but not at)
the EOF. Patches solve possible problems derived from this.

Miquel

[Liborigin-devel] [PATCH 1/2] Fix bug when reading Note windows in files without Note windows.

[Miquel G. <gbm...@gm...>]

---
 Origin610Parser.cpp |    8 ++++++++
 Origin700Parser.cpp |    5 +++++
 Origin800Parser.cpp |    8 ++++++++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/Origin610Parser.cpp b/Origin610Parser.cpp
index ddacd8f..2b83d58 100644
--- a/Origin610Parser.cpp
+++ b/Origin610Parser.cpp
@@ -436,6 +436,11 @@ bool Origin610Parser::parse()
 	}
 
 	readNotes();
+	// If file has no Note windows, last function will read to EOF,
+	// skipping the info for ResultsLog and ProjectTree.
+	// As we know there is always a ResultsLog window after the Note
+	// windows, we better rewind to the start of Notes
+	file.seekg(POS, ios_base::beg);
 	readResultsLog();
 
 	file.seekg(1 + 4*5 + 0x10 + 1, ios_base::cur);
@@ -454,6 +459,9 @@ bool Origin610Parser::parse()
 void Origin610Parser::readNotes()
 {
 	unsigned int pos = findStringPos("@");
+	// if we are at end of file don't try to read a Note
+	if (!(pos < d_file_size))
+		return;
 	file.seekg(pos, ios_base::beg);
 
 	unsigned int sectionSize;
diff --git a/Origin700Parser.cpp b/Origin700Parser.cpp
index 8f70ed0..029e8ca 100644
--- a/Origin700Parser.cpp
+++ b/Origin700Parser.cpp
@@ -419,6 +419,11 @@ bool Origin700Parser::parse()
 	}
 
 	readNotes();
+	// If file has no Note windows, last function will read to EOF,
+	// skipping the info for ResultsLog and ProjectTree.
+	// As we know there is always a ResultsLog window after the Note
+	// windows, we better rewind to the start of Notes
+	file.seekg(POS, ios_base::beg);
 	readResultsLog();
 
 	file.seekg(1 + 4*5 + 0x10 + 1, ios_base::cur);
diff --git a/Origin800Parser.cpp b/Origin800Parser.cpp
index 6dfed47..374e0a3 100644
--- a/Origin800Parser.cpp
+++ b/Origin800Parser.cpp
@@ -447,6 +447,11 @@ bool Origin800Parser::parse()
 	}
 
 	readNotes();
+	// If file has no Note windows, last function will read to EOF,
+	// skipping the info for ResultsLog and ProjectTree.
+	// As we know there is always a ResultsLog window after the Note
+	// windows, we better rewind to the start of Notes
+	file.seekg(POS, ios_base::beg);
 	readResultsLog();
 
 	file.seekg(1 + 4*5 + 0x10 + 1, ios_base::cur);
@@ -465,6 +470,9 @@ bool Origin800Parser::parse()
 void Origin800Parser::readNotes()
 {
 	unsigned int pos = findStringPos(notes_pos_mark);
+	// if we are at end of file don't try to read a Note
+	if (!(pos < d_file_size))
+		return;
 	file.seekg(pos, ios_base::beg);
 
 	unsigned int sectionSize;
-- 
1.7.3

[Liborigin-devel] [PATCH 2/2] Fix bug when reading column info.

[Miquel G. <gbm...@gm...>]

---
 Origin800Parser.cpp |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/Origin800Parser.cpp b/Origin800Parser.cpp
index 374e0a3..3181a9b 100644
--- a/Origin800Parser.cpp
+++ b/Origin800Parser.cpp
@@ -844,7 +844,9 @@ void Origin800Parser::readColumnInfo(int spread, int i)
 	LOG_PRINT(logfile, "		Column %s\n", colName.c_str())
 
 	unsigned int pos = findStringPos(colName);
-	if (file.eof())
+	// even if colName was not found we are not yet at eof, 
+	// just before it. We cannot use file.eof()
+	if (!(pos < d_file_size))
 		return;
 
 	file.seekg(pos - 1, ios_base::beg);
-- 
1.7.3

Re: [Liborigin-devel] Bug fixes

[Knut F. <Knu...@gm...>]

[2010-11-10 00:02] Miquel Garriga <gbm...@gm...>:
> Function Origin800Parser::findStringPos when the passed string
> is not found leaves the 'get pointer' of file close to (but not at)
> the EOF. Patches solve possible problems derived from this.

To be precise, the patches resolve *some* of the problems caused by this. 
There are definitely more bugs lurking in the general vicinity.

I'd say the problem is really findStringPos(). If it finds the string searched 
for, it seeks to the position where the search started and returns the 
position where the match occured, but if the string is *not* found, it leaves 
the file almost (but not quite) at EOF and returns that position. Nobody, 
apparantly including the authors of that code, would expect that. Just search 
through the callers of findStringPos and see how many of them handle the case 
of not finding the string correctly (without your patches, I'm counting zero of 
them).

What findStringPos() *should* do is guarantee that the read position does not 
change (i.e. seekg(startPos) before any return) and clearly indicate failure. 
There are different conventions for that (-1, a special value a la 
std::string::npos, returning bool or error code and writing the actual result 
to a pointer as offered by the GSL, throwing an exception). I think it would 
help if we could try to stick to one such convention.

Knut

Re: [Liborigin-devel] Bug fixes

[Miquel G. <gbm...@gm...>]

2010/11/10 Knut Franke <Knu...@gm...>:
>
> What findStringPos() *should* do is guarantee that the read position does not
> change (i.e. seekg(startPos) before any return) and clearly indicate failure.
> There are different conventions for that (-1, a special value a la
> std::string::npos, returning bool or error code and writing the actual result
> to a pointer as offered by the GSL, throwing an exception). I think it would
> help if we could try to stick to one such convention.

Yes, you are right, but, since the generalized parser will not  need
findStringPos()
at all, I saw no need to enhance it. You should not return -1 in an
unsigned int,
unless you use it to represent the largest available unsigned int. On
the other hand
 <cstring> header uses  string::npos = static_cast<size_type>(-1).
In the case of findStringPos, I agree that a better patch would be:

size_t Origin800Parser::findStringPos(const string& name)
{
...
return std::string::npos;
}
since to represent a position is what size_t is designed for.

However, I would better spend time in the generalized parser that
finding a good  patch
for each bug in OriginNNNParser.

Miquel

Re: [Liborigin-devel] Bug fixes

[Knut F. <Knu...@gm...>]

Okay; since the patches are definitely an improvement and even include some 
comments explaining the weird behaviour, I won't let better be the enemy of 
good. Committed.

However, this bug raises a more general question to keep in mind for the 
generalized parser:

[2010-11-10 09:25] Miquel Garriga <gbm...@gm...>:
> 2010/11/10 Knut Franke <Knu...@gm...>:
> > What findStringPos() *should* do is guarantee that the read position does
> > not change (i.e. seekg(startPos) before any return) and clearly indicate
> > failure. There are different conventions for that (-1, a special value a
> > la std::string::npos, returning bool or error code and writing the
> > actual result to a pointer as offered by the GSL, throwing an
> > exception). I think it would help if we could try to stick to one such
> > convention.
> 
> Yes, you are right, but, since the generalized parser will not  need
> findStringPos()
> at all,

Maybe; but it will definitely need some kind of error handling for cases where 
a (possibly corrupted, or unsupported) file is not parsable by some (internal) 
function. Having a set convention would help avoid bugs like these, where 
findStringPos() returns the position of the last byte in the file in case of 
failure, while callers variously expect -1, expect the read position to be at 
EOF or ignore this case altogether.

Personally I think that exceptions would provide a good error-signalling 
convention. Maybe this particular example is a corner case, since very similar 
methods in the STL use the magic value std::string::npos.

> You should not return -1 in an
> unsigned int,

Of course, with that convention the return type needs to be changed.

> However, I would better spend time in the generalized parser that
> finding a good  patch
> for each bug in OriginNNNParser.

Fair enough.

Knut