Bug#773818: Crashes on start
On Tue, Dec 30, 2014 at 10:49:34AM +0100, Andreas Henriksson wrote:
> Hello Adam Majer!
Hello!
> Stumbled across your bug report while browsing over release-critical ones...
>
> I haven't even looked at the lpe source, but just from looking at the
> hunk included as context in your patch it looks like the source
> could really use some wider review then just targeted fixes.
That's quite correct. lpe hasn't really been updated in a decade. But
it's still a very simple editor I prefer over vim or emacs for simple
tasks.
- Ctrl-C close without saving
- Ctrl-X close and save
- Ctrl-V Ctrl-V/A/S - pass buffer through shell command, awk, or sed
Can't get faster than that.
> On Tue, Dec 23, 2014 at 09:34:20AM -0600, Adam Majer wrote:
> [...]
> > diff -u lpe-1.2.7/src/buffer.c lpe-1.2.7/src/buffer.c
> > --- lpe-1.2.7/src/buffer.c 2014-06-23 22:53:33.582593198 -0500
> > +++ lpe-1.2.7/src/buffer.c 2014-12-23 09:08:54.888625050 -0600
> > @@ -158,8 +158,8 @@
> > int (*accept) (buffer *);
> >
>
> Consider the case where strlen(ent->d_name) == basename_len.
>
> > if (strlen(ent->d_name) > basename_len) {
> This should probably use >= .
No. This already includes null when accounting for basename_len. If
strlen == basename_len then there is enough room in the buffer. Output
is just before the \0 that is already allocated.
That is also why that +1 to account for \0 in the calculation was in
the wrong spot and caused issues on next loop when filename could be
+1 character longer. And those happen,
cppmode.so
htmlmode.so
> As mentioned, I haven't looked at the full source so I might very well
> be missing something. As I understood it this is the second attempt at
> fixing an issue here. Possibly a wider review of how to avoid
> off-by-one in the entire source could be useful.
In this case this is the problem. I think it was caused by trying to
be "fancy" and retaining old code that used fixed length array and
then apply it to dynamic array. Better approach would have been to
simplify all those concatenations in one spot instead of all over the
loop.
You are 100% correct. lpe would benefit from overall update.
Thanks for looking at this.
- Adam
--
Adam Majer
ad...@zombino.com
--
To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#773818: Crashes on start
Hello Adam Majer!
Stumbled across your bug report while browsing over release-critical ones...
I haven't even looked at the lpe source, but just from looking at the
hunk included as context in your patch it looks like the source
could really use some wider review then just targeted fixes.
On Tue, Dec 23, 2014 at 09:34:20AM -0600, Adam Majer wrote:
[...]
> diff -u lpe-1.2.7/src/buffer.c lpe-1.2.7/src/buffer.c
> --- lpe-1.2.7/src/buffer.c2014-06-23 22:53:33.582593198 -0500
> +++ lpe-1.2.7/src/buffer.c2014-12-23 09:08:54.888625050 -0600
> @@ -158,8 +158,8 @@
> int (*accept) (buffer *);
>
Consider the case where strlen(ent->d_name) == basename_len.
> if (strlen(ent->d_name) > basename_len) {
This should probably use >= .
The strlen() function calculates the length of the string s,
=>excluding the terminating null byte ('\0').
> -basename_len = strlen(ent->d_name) + 1;
> -name = realloc(name, (basename-name) + basename_len);
> +basename_len = strlen(ent->d_name);
> +name = realloc(name, (basename-name) + basename_len + 1);
> basename = name + basename_off;
> }
> strcpy (basename, ent->d_name);
The strcpy() function copies the string pointed to by src,
=>including the terminating null byte ('\0'), ...
As mentioned, I haven't looked at the full source so I might very well
be missing something. As I understood it this is the second attempt at
fixing an issue here. Possibly a wider review of how to avoid
off-by-one in the entire source could be useful.
Regards,
Andreas Henriksson
--
To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#773818: Crashes on start
Package: lpe
Version: 1.2.7-1
Severity: grave
There is a off-by-1 buffer overflow in my buffer overflow fix. While
scanning for plugins, the length of available basename is stored that
includes \0, and then in another loop it is tested against
strlen(basename) that clearly does not include trailing \0.
Workaround is to either remove all the plugins, or change the order in
which they are returned by the file system (kind of tricky!), or apply
the patch,
diff -u lpe-1.2.7/src/buffer.c lpe-1.2.7/src/buffer.c
--- lpe-1.2.7/src/buffer.c 2014-06-23 22:53:33.582593198 -0500
+++ lpe-1.2.7/src/buffer.c 2014-12-23 09:08:54.888625050 -0600
@@ -158,8 +158,8 @@
int (*accept) (buffer *);
if (strlen(ent->d_name) > basename_len) {
-basename_len = strlen(ent->d_name) + 1;
-name = realloc(name, (basename-name) + basename_len);
+basename_len = strlen(ent->d_name);
+name = realloc(name, (basename-name) + basename_len + 1);
basename = name + basename_off;
}
strcpy (basename, ent->d_name);
-- System Information:
Debian Release: jessie/sid
APT prefers unstable
APT policy: (500, 'unstable'), (50, 'experimental')
Architecture: amd64 (x86_64)
Foreign Architectures: i386
Kernel: Linux 3.16.0-4-amd64 (SMP w/4 CPU cores)
Locale: LANG=en_CA.UTF-8, LC_CTYPE=en_CA.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Versions of packages lpe depends on:
ii libc62.19-13
ii libncurses5 5.9+20140913-1
ii libslang22.3.0-2
ii libtinfo55.9+20140913-1
lpe recommends no packages.
lpe suggests no packages.
-- no debconf information
--
To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
