Re: [hackers] [sbase] [patch] ed: standards compliance, manpage, double free and infinite loop fix

[Laslo Hunhold]

On Sun, 9 Oct 2016 23:20:20 +
Thomas Mannay  wrote:

Hey Thomas,

> It took a while longer than I'd planned, but I've gone through and
> redone my patches. I found a second source of an infinite loop to do
> with joining where a variable would be -1 and the counter started at
> 0, so could never equal it. I also added a safety guard on j to
> prevent someone passing it a 0 address, which would cause a segfault.
> 
> Hopefully all's in order and my editor saved with tabs this time.

thanks for your hard work! I applied your patches and fixed a few
things with the manpage you submitted using mandoc -Tlint.

Cheers

Laslo

-- 
Laslo Hunhold 

Re: [hackers] [sbase] [patch] ed: standards compliance, manpage, double free and infinite loop fix

[Thomas Mannay]

It took a while longer than I'd planned, but I've gone through and redone my
patches. I found a second source of an infinite loop to do with joining
where a variable would be -1 and the counter started at 0, so could never
equal it. I also added a safety guard on j to prevent someone passing it a
0 address, which would cause a segfault.

Hopefully all's in order and my editor saved with tabs this time.

Thanks,
Thomas

--
Thomas Mannay 
>From e27ecb4eaf04fcec871c4eb5319f1c14bc151d3d Mon Sep 17 00:00:00 2001
From: Thomas Mannay 
Date: Sun, 9 Oct 2016 23:10:20 +
Subject: [PATCH 1/5] ed: remove infinite loops in join() and getindex()

---
 ed.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/ed.c b/ed.c
index 184ed30..f552234 100644
--- a/ed.c
+++ b/ed.c
@@ -192,7 +192,9 @@ getindex(int line)
 	struct hline *lp;
 	int n;
 
-	for (n = 0, lp = zero; n != line; ++n)
+	if (line == -1)
+		line = 0;
+	for (n = 0, lp = zero; n != line; n++)
 		lp = zero + lp->next;
 
 	return lp - zero;
@@ -806,9 +808,11 @@ join(void)
 	static char *s;
 
 	free(s);
-	for (s = NULL, i = line1; i <= line2; i = nextln(i)) {
+	for (s = NULL, i = line1;; i = nextln(i)) {
 		for (t = gettxt(i); (c = *t) != '\n'; ++t)
 			s = addchar(*t, s, , );
+		if (i == line2)
+			break;
 	}
 
 	s = addchar('\n', s, , );
-- 
2.10.0

>From 5069287a04fb182e83269b86b95f01169ae36233 Mon Sep 17 00:00:00 2001
From: Thomas Mannay 
Date: Sun, 9 Oct 2016 23:11:01 +
Subject: [PATCH 2/5] ed: giving j only one address does nothing

---
 ed.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/ed.c b/ed.c
index f552234..4ab40bc 100644
--- a/ed.c
+++ b/ed.c
@@ -1174,9 +1174,8 @@ repeat:
 	case 'j':
 		chkprint(1);
 		deflines(curln, curln+1);
-		if (!line1)
-			goto bad_address;
-		join();
+		if (line1 != line2 && curln != 0)
+	  		join();
 		break;
 	case 'z':
 		if (nlines > 1)
-- 
2.10.0

>From 90f8b32b4dac7553882fce39094a338a399a1d44 Mon Sep 17 00:00:00 2001
From: Thomas Mannay 
Date: Sun, 9 Oct 2016 23:12:46 +
Subject: [PATCH 3/5] ed: place newly joined lines correctly

---
 ed.c | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/ed.c b/ed.c
index 4ab40bc..d123814 100644
--- a/ed.c
+++ b/ed.c
@@ -299,13 +299,17 @@ undo(void)
 }
 
 static void
-inject(char *s)
+inject(char *s, int j)
 {
 	int off, k, begin, end;
 
-	begin = getindex(curln);
-	end = getindex(nextln(curln));
-
+	if (j) {
+		begin = getindex(curln-1);
+		end = getindex(nextln(curln-1));
+	} else {
+		begin = getindex(curln);
+		end = getindex(nextln(curln));
+	}
 	while (*s) {
 		k = makeline(s, );
 		s += off;
@@ -636,7 +640,7 @@ doread(char *fname)
 			s[n-1] = '\n';
 			s[n] = '\0';
 		}
-		inject(s);
+		inject(s, 0);
 	}
 	if (optdiag)
 		printf("%zu\n", cnt);
@@ -753,7 +757,7 @@ append(int num)
 	while (getline(, , stdin) > 0) {
 		if (*s == '.' && s[1] == '\n')
 			break;
-		inject(s);
+		inject(s, 0);
 	}
 	free(s);
 }
@@ -818,7 +822,7 @@ join(void)
 	s = addchar('\n', s, , );
 	s = addchar('\0', s, , );
 	delete(line1, line2);
-	inject(s);
+	inject(s, 1);
 	free(s);
 }
 
@@ -845,7 +849,7 @@ copy(int where)
 	curln = where;
 
 	for (i = line1; i <= line2; ++i)
-		inject(gettxt(i));
+		inject(gettxt(i), 0);
 }
 
 static void
@@ -1025,7 +1029,7 @@ subline(int num, int nth)
 	addpost(, , );
 	delete(num, num);
 	curln = prevln(num);
-	inject(s);
+	inject(s, 0);
 }
 
 static void
-- 
2.10.0

>From 977008c84e74f0c0fc1c4655ccd0b35920f0b5a3 Mon Sep 17 00:00:00 2001
From: Thomas Mannay 
Date: Sun, 9 Oct 2016 23:13:13 +
Subject: [PATCH 4/5] ed: remove double free in join()

---
 ed.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ed.c b/ed.c
index d123814..de5321f 100644
--- a/ed.c
+++ b/ed.c
@@ -809,9 +809,8 @@ join(void)
 	int i;
 	char *t, c;
 	size_t len = 0, cap = 0;
-	static char *s;
+	char *s;
 
-	free(s);
 	for (s = NULL, i = line1;; i = nextln(i)) {
 		for (t = gettxt(i); (c = *t) != '\n'; ++t)
 			s = addchar(*t, s, , );
-- 
2.10.0

>From 7740d822cd5d52fd0abd5c8350987345c610309a Mon Sep 17 00:00:00 2001
From: Thomas Mannay 
Date: Sun, 9 Oct 2016 23:14:54 +
Subject: [PATCH 5/5] ed: add manpage

---
 ed.1 | 210 ++-
 1 file changed, 209 insertions(+), 1 deletion(-)

diff --git a/ed.1 b/ed.1
index 93e3012..50351e2 100644
--- a/ed.1
+++ b/ed.1
@@ -6,4 +6,212 @@
 .Nd text editor
 .Sh SYNOPSIS
 .Nm
-is the standard text editor.
+.Op Fl s
+.Op Fl p Ar string
+.Op Ar file
+.Sh DESCRIPTION
+.Nm
+is the standard text editor. It performs line-oriented operations on a buffer;
+The buffer's contents are manipulated in command mode and text is written to the
+buffer in input mode. Command mode is the default. To exit input mode 

Re: [hackers] [sbase] [patch] ed: standards compliance, manpage, double free and infinite loop fix

[Laslo Hunhold]

On Sun, 9 Oct 2016 21:40:55 +
Thomas Mannay  wrote:

Hey Thomas,

> > - the code within join() is not tab- but space-indented
> And here I thought I had set my editor to indent tabs...

this can happen, no problem.

> > - can getindex(curln-1) underflow? (if curln = 0)
> iirc currln can't be 0 since addresses start at 1 in ed. I've
> experienced no issues with joining a set of addresses beginning with
> the first line.

Alright, that sounds like a reasonable point. But what if curln is 1,
then the line index passed to getindex is "0". Is that a valid line
index?

> > - what's the purpose of the free(s)?
> The original code had two frees, which I assume was due to a static
> char pointer. This would cause a double free if you did two join
> commands in a row. The free at the end of the function is required
> because addchar returns a manually allocated char pointer.

Thanks for elaborating! I somehow forgot to notice the "static" for s.

> > - can you give an example in the patch-description where this
> >   infinite loop occurs?
> I'm unsure where the patch description would be, but it occurs when
> you issue 1,2j when there are only two lines in the buffer. Doing
> that on the unpatched ed causes it to stop accepting commands until
> given an interrupt due to an infinite loop.

Alright, so the patch description is the "commit description" so to
say. It would be nice to have a minimal working example so people know
what the patch fixes and test it quickly and easily.

> > - Do we really need an "EXTENDED DESCRIPTION"
> Yes, it's one of the standard manpage subheadings and allowed me to
> properly document ed. It felt like The Right Thing since correct
> documentation is as important as correct code. It's actually how I
> found the bugs I fixed. :^)

Fair point!

> > - Please check the other manpages for the standard format of
> >   the STANDARDS section
> Have amended this with the attached patch.

Alright, thanks!
 
> Should I redo those patches to account for changing commit messages
> to more active forms?

I would welcome this!

Cheers

Laslo

-- 
Laslo Hunhold 

Re: [hackers] [sbase] [patch] ed: standards compliance, manpage, double free and infinite loop fix

[Thomas Mannay]

I only went and forgot to attach the patch to ed.1...

---
 ed.1 | 5 +
 1 file changed, 5 insertions(+)

diff --git a/ed.1 b/ed.1
index c9f8a68..50351e2 100644
--- a/ed.1
+++ b/ed.1
@@ -205,6 +205,11 @@ characters. When
 returns a '!' is printed. The dot is unchanged.
 .El
 .Sh STANDARDS
+The
+.Nm
+utility is compliant with the
+.St -p1003.1-2013
+specification, except where noted here:
 g and v operate on single commands rather than lists delimited with '\\'.
 e, E, r, w, and W commands cannot accept shell escapes.
 .Sh SEE ALSO
-- 
2.10.0

--
Thomas Mannay 

Re: [hackers] [sbase] [patch] ed: standards compliance, manpage, double free and infinite loop fix

[Thomas Mannay]

Thank you for the feedback.

> - change the name to "ed: place newly-joined lines correctly"
Will bear in mind for future patches that I should use more active descriptions.
> - the code within join() is not tab- but space-indented
And here I thought I had set my editor to indent tabs...
> - can getindex(curln-1) underflow? (if curln = 0)
iirc currln can't be 0 since addresses start at 1 in ed. I've experienced no
issues with joining a set of addresses beginning with the first line.
> - what's the purpose of the free(s)?
The original code had two frees, which I assume was due to a static char 
pointer.
This would cause a double free if you did two join commands in a row. The free 
at
the end of the function is required because addchar returns a manually allocated
char pointer.
> - can you give an example in the patch-description where this
>   infinite loop occurs?
I'm unsure where the patch description would be, but it occurs when you issue
1,2j when there are only two lines in the buffer. Doing that on the unpatched ed
causes it to stop accepting commands until given an interrupt due to an infinite
loop.
> - Do we really need an "EXTENDED DESCRIPTION"
Yes, it's one of the standard manpage subheadings and allowed me to properly
document ed. It felt like The Right Thing since correct documentation is as
important as correct code. It's actually how I found the bugs I fixed. :^)
> - Please check the other manpages for the standard format of
>   the STANDARDS section
Have amended this with the attached patch.

Should I redo those patches to account for changing commit messages to more
active forms?

Thank you,
Thomas

--
Thomas Mannay 

Re: [hackers] [sbase] [patch] ed: standards compliance, manpage, double free and infinite loop fix

[Laslo Hunhold]

On Thu, 6 Oct 2016 11:04:17 +
Thomas Mannay  wrote:

Hey Thomas,

> Resubmission of my patchset for ed, albeit much better formatted so
> as to ease reading them.

as promised, I will give you some feedback on your patches you sent in.

### 0001-ed-newly-joined-lines-are-placed-correctly.patch

- change the name to "ed: place newly-joined lines correctly"
- the code within join() is not tab- but space-indented
- can getindex(curln-1) underflow? (if curln = 0)
- what's the purpose of the free(s)?

### 0002-ed-if-only-one-address-is-given-to-j-do-nothing.patch

### 0003-ed-fix-double-free-and-infinite-loop-in-join.patch

- can you give an example in the patch-description where this
  infinite loop occurs?

### 0004-ed-wrote-manpage.patch

- change the name to "ed: add a manpage"
- Do we really need an "EXTENDED DESCRIPTION"
- Please check the other manpages for the standard format of
  the STANDARDS section

Thanks for your submissions!

Cheers

Laslo

-- 
Laslo Hunhold 

Re: [hackers] [sbase] [patch] ed: standards compliance, manpage, double free and infinite loop fix

[Greg Reagle]

On Fri, Oct 7, 2016, at 09:43, Laslo Hunhold wrote:
> one thing that has caught my attention is the titling of the patches.
> It is better to use the active form rather than passive form, so my
> suggestions for renaming would be
> 
> "ed: newly joined lines are placed correctly"
> -> "ed: place newly joined lines correctly"
> 
> "ed: wrote manpage"
> -> "add a manpage"

These are both active.  "wrote" is the past tense and "add" is not, but
neither are passive.

Re: [hackers] [sbase] [patch] ed: standards compliance, manpage, double free and infinite loop fix

[Laslo Hunhold]

On Thu, 6 Oct 2016 11:04:17 +
Thomas Mannay  wrote:

Hey Thomas,

> 

one thing that has caught my attention is the titling of the patches.
It is better to use the active form rather than passive form, so my
suggestions for renaming would be

"ed: newly joined lines are placed correctly"
-> "ed: place newly joined lines correctly"

"ed: wrote manpage"
-> "add a manpage"

Feedback on the content will come Sunday evening.

Cheers

Laslo

-- 
Laslo Hunhold 

Re: [hackers] [sbase] [patch] ed: standards compliance, manpage, double free and infinite loop fix

[Laslo Hunhold]

On Thu, 6 Oct 2016 11:04:17 +
Thomas Mannay  wrote:

Hey Thomas,

> Resubmission of my patchset for ed, albeit much better formatted so
> as to ease reading them.

thank you very much for your patches! I'll take a look at them Sunday
evening and give you some feedback. They already look very good!

Cheers

Laslo

-- 
Laslo Hunhold