cmd_pipe() is bugged (invalid descriptor -1)

[kyx0r]

cmd.c line 111,112 may have -1 passed in which is invalid. Add if statements to prevent this.

[aligrudi]

Hi,

Kyryl @.***> wrote:

cmd.c line 111,112 may have -1 passed in which is invalid. Add if statements to prevent this.

I am not concerned about close(-1), as it is harmless. However, a closed file descriptor may be reused and closed unintentionally. This however never happens in Neatvi, because no file descriptor is allocated in the while loop calling poll(). More details:

Calling close() in the first two conditions of the while loop of cmd_pipe() is necessary, because some programs wait for EOF. These descriptors are closed once more after the loop; if these file descriptors are allocated after the first closing (which as I mentioned never happens in Neatvi), there would be a problem.

The following patch probably fixes it; but I does not seem very clean. What do you think?

By the way, to search for simple patterns (as in ^a command) in large files, I added rstr.c. It is much faster than the regex search.

Thanks, Ali

diff --git a/cmd.c b/cmd.c index be9c1c4..166a0fd 100644 --- a/cmd.c +++ b/cmd.c @@ -82,16 +82,20 @@ char cmd_pipe(char cmd, char *ibuf, int iproc, int oproc) int ret = read(fds[0].fd, buf, sizeof(buf)); if (ret > 0) sbuf_mem(sb, buf, ret);

• if (ret <= 0)
• if (ret <= 0) { close(fds[0].fd);
• fds[0].fd = -1;
• } continue; } if (fds[1].revents & POLLOUT) { int ret = write(fds[1].fd, ibuf + nw, slen - nw); if (ret > 0) nw += ret;
• if (ret <= 0 || nw == slen)
• if (ret <= 0 || nw == slen) { close(fds[1].fd);
• fds[1].fd = -1;
• } continue; } if (fds[2].revents & POLLIN) { @@ -101,15 +105,19 @@ char cmd_pipe(char cmd, char *ibuf, int iproc, int oproc) if ((unsigned char) buf[i] == TK_CTL('c')) kill(pid, SIGINT); }
• if (fds[0].revents & (POLLERR | POLLHUP | POLLNVAL))
• if (fds[0].revents & (POLLERR | POLLHUP | POLLNVAL)) {
• close(fds[0].fd); fds[0].fd = -1;
• if (fds[1].revents & (POLLERR | POLLHUP | POLLNVAL))
• }
• if (fds[1].revents & (POLLERR | POLLHUP | POLLNVAL)) {
• close(fds[1].fd); fds[1].fd = -1;
• } if (fds[2].revents & (POLLERR | POLLHUP | POLLNVAL)) fds[2].fd = -1; }
• close(ifd);
• close(ofd);
• close(fds[0].fd);
• close(fds[1].fd); waitpid(pid, NULL, 0); if (!iproc) { term_init();

[kyx0r]

Ali Gholami Rudi @.***> wrote:

Hi,

Kyryl @.***> wrote:

cmd.c line 111,112 may have -1 passed in which is invalid. Add if statements to prevent this.

I am not concerned about close(-1), as it is harmless. However, a closed file descriptor may be reused and closed unintentionally. This however never happens in Neatvi, because no file descriptor is allocated in the while loop calling poll(). More details:

Calling close() in the first two conditions of the while loop of cmd_pipe() is necessary, because some programs wait for EOF. These descriptors are closed once more after the loop; if these file descriptors are allocated after the first closing (which as I mentioned never happens in Neatvi), there would be a problem.

The following patch probably fixes it; but I does not seem very clean. What do you think?

By all means, as long is it does not cause problems on some other platform. I am not sure if it's possible to have descriptor with value -1 but it just seemed odd to me.

By the way, to search for simple patterns (as in ^a command) in large files, I added rstr.c. It is much faster than the regex search.

Seen it, pretty cool. I benched it and it was about 3 times faster than searching through pikevm using simple pattern like "abc". I don't know how it's implemented, but if I get to it I might add it into nextvi. Though nextvi does not store keyword string in static memory it always stores compiled regular expression in a global varible pointer, so if I wanted to add rstr.c it would be quite some extra work.

Currently pikevm has 1 more optimization possible that I did not do yet. It only really affects complex regexes with many alts, submatches, transitions. The duplicate state elimination can be achieved in O(1) time at the expense of space using the sparse array datastructure and abusing/taking advantage of uninialized memory. see this: https://research.swtch.com/sparse One of the coolest C tricks ever

Thanks, Kyryl