Unallocated pointer freed in plpftp

[kapfab]

I was trying to recompile the latest changes on macOS and got an issue with plpftp when using the ls command:

malloc: *** error for object 0x195fc95d2: pointer being freed was not allocated

The issue comes from commit https://github.com/rrthomas/plptools/commit/5eb62f2745f12dfe790fe5e1bdc0fe372cc36512, adding line 934 to plpftp/ftp.cc:

        free(f1);

[kapfab]

Also, cd .. isn’t working anymore. Probably due to the related changes in the same commit.

[rrthomas]

Thanks, looks like this was overambitious refactoring on my part. Since the spirit of the commit is good, I shall try to make a partial version, as a lot of it is "obviously right".

[kapfab]

Let me know when you want me to test your fixes, that’s way easier with a device at hand!

[rrthomas]

I've just pushed a straightforward revert of the offending commit. Please could you check that things now work again? This is to make sure that it was indeed this commit and only this commit that was the problem. If that turns out to be the case, I can then proceed to redoing the commit more carefully.

[kapfab]

I confirm both issues are gone with the revert.

[rrthomas]

Many thanks. I have now re-added what I consider the "easy" cases of that commit. This affects the following commands: ren, cp, touch, test, gattr, sattr, gtime, get, mget, put, del/rm, mkdir, rmdir

Please could you do a quick test of each of these? I think a valid and invalid invocation of each command should suffice: the logic is not so complicated that there are more than 2 cases for any of them.

[rrthomas]

(Probably obvious, but I should perhaps re-state the motivation here: I'm trying to get rid of all the statically-allocated buffers, to avoid potential buffer overflows. The problematic commit addressed the most pervasive and, as it turned out, trickiest case. There are some other, simpler (because more local) cases that I will also address.)

[rrthomas]

I have queued up a re-do of the rest of the faulty commit, as 3 further commits. My plan is to feed them into GitHub one commit at a time and let you test each one. The most complex code is for the commands that deal with directories: ls/dir and cd, which fortunately was redundant, so I've factored it out and commented it: it should now be easier to get right if it's not already good to go.

After that, I will continue with the rest of the static buffers.

[rrthomas]

(Obviously, no hurry!)

[kapfab]

Tested successfully:

• ren
• cp
• touch
• test
• gattr
• sattr
• gtime
• get
• mget
• put
• mput
• del
• rm
• mkdir
• rmdir
• ls
• ls \<dir>
• dir
• dir \<dir>
• cd
• lcd

[rrthomas]

Many thanks! I see you tested all the commands, more or less, including some I didn't change in this commit. For future commits I have changed only one or two specific commands, so it shouldn't be necessary to test everything (maybe reserve that for a final check at the end?).

Next up: I have updated the code for mput.

[kapfab]

mput works fine with your latest change!

[rrthomas]

Thanks again!

Next up: ls, dir and cd are all affected by the next commit.

[kapfab]

There’s an issue with cd .. and the CWD seems to get lost if I cd to a non-existing directory :

Psion dir is: "C:\"
> ls
drw-------          0 Fri Jan  1 12:10:04 1999 System
drw-------          0 Fri Jan  1 12:10:04 1999 Documents
> cd Documents
> ls
-rw-------       9906 Thu Jul 15 14:00:00 1999 Bienvenue
-rw--a----        415 Fri Jan  1 12:10:16 1999 Tableur
-rw--a----        680 Fri Jan  1 12:10:18 1999 Fiches
-rw--a----       1104 Fri Jan  1 12:10:22 1999 Agenda
-rw--a----        768 Fri Jan  1 12:10:36 1999 Calepin
-rw--a----        133 Fri Jan  1 12:10:36 1999 Dessin
-rw--a----        242 Fri Jan  1 12:18:16 1999 Program
> cd ..
> ls
-rw-------       9906 Thu Jul 15 14:00:00 1999 Bienvenue
-rw--a----        415 Fri Jan  1 12:10:16 1999 Tableur
-rw--a----        680 Fri Jan  1 12:10:18 1999 Fiches
-rw--a----       1104 Fri Jan  1 12:10:22 1999 Agenda
-rw--a----        768 Fri Jan  1 12:10:36 1999 Calepin
-rw--a----        133 Fri Jan  1 12:10:36 1999 Dessin
-rw--a----        242 Fri Jan  1 12:18:16 1999 Program
> cd Documents 
Error: no such directory
Keeping original directory "C:\Documents\Documents\"
> ls
Error: no such directory
> 

[rrthomas]

Great, thanks. Looks like this is where I went wrong before. I'll have a closer look.

[rrthomas]

I found it, I think! I have updated the last commit, and I think the bug in cd .. actually caused the following problem too. So, same test as before: ls, dir, cd. (Also please try both ls and cd with no argument.)

[kapfab]

ls/dir with and without arguments and cd without arguments work fine but there are still issues with cd:

• cd .. does nothing when .. is the root directory,
• cd non_existing_dir triggers an Error: no such directory but sets the CWD to this non_existing_dir (and subsequent ls gives an Error: no such directory).

[rrthomas]

Thanks for this, and particularly for your patience. I was fairly sure that there was not an independent bug for cd nonexistent, but it looks like there is. As for cd .., sounds like we're getting there.

I shall endeavour to find some way to debug this without being able to use a real device, so that I don't have to keep asking you "is it fixed yet?".

[kapfab]

I think I got it. In epoc_dirname(const char *path), the trailing slash is ‘removed' (p gets the \ position, two chars only from the start of the string)

    /* Skip backwards to next slash. */
    while ((p > f1) && (*p != '/') && (*p != '\\'))
    p--;

so

    /* If we have just a drive letter, colon and slash, keep the entire string. */
    if (p - f1 < 3)
    p = f1 + strlen(f1);

should be changed to something like

    /* If we don’t have at least a drive letter and colon, keep the current path. */
    if (p - f1 < 2)
    p = f1 + strlen(f1);

Can’t test it right now, but should work.

EDIT: Updated my explanation to include the right code snippet. This change solves the cd .. issue when the parent directory is the root directory.

Now there’s still the non-existent directory issue to be fixed.

[kapfab]

Changing

        char *f1 = epoc_dir_from(argv[1]);
        strcpy(psionDir, f1);
        uint32_t tmp;
        if ((res = a.dircount(f1, tmp)) != rfsv::E_PSI_GEN_NONE) {
            cerr << _("Error: ") << res << endl;
            cerr << _("Keeping original directory \"") << psionDir << "\"" << endl;
        }
        free(f1);

to

        char *f1 = epoc_dir_from(argv[1]);
        uint32_t tmp;
        if ((res = a.dircount(f1, tmp)) != rfsv::E_PSI_GEN_NONE) {
            cerr << _("Error: ") << res << endl;
            cerr << _("Keeping original directory \"") << psionDir << "\"" << endl;
        } else
            strcpy(psionDir, f1);
        free(f1);

seems to fix the cd nonexistent issue.

[rrthomas]

Many thanks for looking into this! I'll have a look at your suggestions and see if I can better understand myself what is going on.

[rrthomas]

I think I got it. In epoc_dirname(const char *path), the trailing slash is ‘removed' (p gets the \ position, two chars only from the start of the string)

Correct, so at this point, p == f1 + 2 (it points to the third character).

    /* If we have just a drive letter, colon and slash, keep the entire string. */
    if (p - f1 < 3)
  p = f1 + strlen(f1);

Hence, this test is correct: when p == f1 + 2, p - f1 < 3 is true.

    /* If we don’t have at least a drive letter and colon, keep the current path. */
    if (p - f1 < 2)
  p = f1 + strlen(f1);

Nope. This condition should never be true.

I've worked out what's going on, and I can see how my code changes introduced this bug! But the new code is simpler and fixes compiler warnings, and I think there is in fact an easy fix:

    /* If we have just a drive letter, colon and slash, keep exactly that. */
    if (p - f1 < 3)
  p = f1 + 3;

The problem was that the old code had by this point truncated the string by storing a NUL at p. But we don't truncate the string until the next line! However, we know exactly how much string we want to keep (3 chars), so we can simply do that.

[rrthomas]

Changing

      char *f1 = epoc_dir_from(argv[1]);
      strcpy(psionDir, f1);
      uint32_t tmp;
      if ((res = a.dircount(f1, tmp)) != rfsv::E_PSI_GEN_NONE) {
          cerr << _("Error: ") << res << endl;
          cerr << _("Keeping original directory \"") << psionDir << "\"" << endl;
      }
      free(f1);

to

      char *f1 = epoc_dir_from(argv[1]);
      uint32_t tmp;
      if ((res = a.dircount(f1, tmp)) != rfsv::E_PSI_GEN_NONE) {
          cerr << _("Error: ") << res << endl;
          cerr << _("Keeping original directory \"") << psionDir << "\"" << endl;
      } else
          strcpy(psionDir, f1);
      free(f1);

seems to fix the cd nonexistent issue.

I have a different fix for this, as part of making psionDir dynamically-allocated.

[rrthomas]

I've updated GitHub again with my fix for epoc_dirname. Test when you can!

[rrthomas]

I have now rejigged some commits so you should now have all the changes relevant to ls/dir and cd. Sorry for the confusion. I also snuck in a commit that only removes unused code. This means that the number of commits left to test after this is smaller than it was.

[kapfab]

Hence, this test is correct: when p == f1 + 2, p - f1 < 3 is true.

It is true and it shouldn't be because in this configuration we don’t want p to go back to f1 + strlen(f1) (function would then return C:\Documents) but rather keep p as it is (in order to return C:) — the ending \ did not need to be part of the returned value as it is appended by the calling function (epoc_dir_from) if missing.

Anyway, with your latest changes, we don't go back to f1 + strlen(f1) anymore but to f1 + 3, so this gives the same results with a different logic.

I’ve tested both approaches work fine.

[kapfab]

There is a case that’s not handled correctly (but I don’t think it has ever been): after using cd with an absolute path, all the cd .. logic fails.

For example, if the path is \System\ then cd .. will try to reach \Sy (if the path is \ then p even points 2 chars outside f1’s memory area).

There should be a dedicated test to prefix absolute paths with the first two chars of psionDir. But I'll probably open another issue for this.

[rrthomas]

the ending \ did not need to be part of the returned value as it is appended by the calling function (epoc_dir_from) if missing.

Aha, that's what made it work! Nonetheless I think that epoc_dirname should maintain the final slash too, to avoid confusion.

[rrthomas]

There is a case that’s not handled correctly (but I don’t think it has ever been): after using cd with an absolute path, all the cd .. logic fails.

For example, if the path is \System\ then cd .. will try to reach \Sy (if the path is \ then p even points 2 chars outside f1’s memory area).

There should be a dedicated test to prefix absolute paths with the first two chars of psionDir. But I'll probably open another issue for this.

Please do!

[kapfab]

By the way, cd nonexistent works fine too now, so this issue can be closed (I let you do this, I don’t know if you want to do some more refactoring).

[rrthomas]

By the way, cd nonexistent works fine too now, so this issue can be closed (I let you do this, I don’t know if you want to do some more refactoring).

Many thanks for confirming. There are more refactoring commits incoming!

[rrthomas]

I've just pushed a change that affects all commands, but equally, so you don't have to test them all again, just one or two. It makes some kind of readline mandatory (but that can be e.g. libedit on BSD), and thereby simplifies the readline code; also removes another static buffer.

[rrthomas]

I have also squashed two commit in history so that later git bisection doesn't hit a random case where cd .. fails.

[rrthomas]

Also, I see I've broken CI, so I'll let you know when it's fixed; no point testing until then.

[rrthomas]

Update: CI is passing again. If you're on macOS, you'll need to set LDFLAGS and CPPFLAGS appropriately (see .github/workflows/c-cpp.yml for a crib) to point to brew's readline. Other than that, I'll repeat that the test for this commit is just to run one or two commands (any will do). It's the command parsing code that has been updated.

[kapfab]

Commands are not recognised. In getCommand, buf is freed but argvcontains pointers to chunks of it.

Also, I don’t think the build would work with libedit as it does not embed history AFAIR. But I’ll try to make sure once the nominal case is OK.

[rrthomas]

Thanks! The current code copes without history APIs, it only requires basic readline.

I've fixed the freeing of argv, I think.

[rrthomas]

OK, I think I've found a simple fix for the bad early free.

[kapfab]

Seems good, thanks!

macOS still comes with BSD’s libedit readline 4.2, so I didn't bother testing anything else than GNU readline…

[rrthomas]

Thanks for confirming! I don't think it's a big deal to require GNU readline on macOS, since, as you observe, the supplied libedit is ancient.

Next up: I've removed a static buffer in filename completion, so just testing some filename completion should be good enough.

I have also looked at my remaining changes. Some of them are too complicated; I will focus on simplifying them to make them more likely to be correct first (or second!) go.

[kapfab]

Filename completion works fine (tested with directories, subdirectories and files).

Also, adding rl_completer_quote_characters = "\""; to the initReadline function makes completion work for names containing spaces:

void ftp::
initReadline(void)
{
    rl_readline_name = "plpftp";
    rl_completion_entry_function = null_completion;
    rl_attempted_completion_function = do_completion; 
    rl_basic_word_break_characters = " \t\n\"\\'`@><=;|&{(";
    rl_completer_quote_characters = "\"";
}

Regarding libedit, I thought this was Apple’s only fault (they do embed a very old version) but it seems that the latest libedit version still defines RL_READLINE_VERSION 0x0402 and still lacks rl_filename_quoting_desired, so… There are so many GNU packages to be installed anyway, requiring GNU readline seems more than reasonable to me.

[rrthomas]

Also, adding rl_completer_quote_characters = "\""; to the initReadline function makes completion work for names containing spaces

Thanks, I've added that in another commit.

Regarding libedit, I thought this was Apple’s only fault (they do embed a very old version) but it seems that the latest libedit version still defines RL_READLINE_VERSION 0x0402

Huh. Oh well, readline is available everywhere.

Next commit pushed: this replaces custom glob-matching code with the standard fnmatch function. (Rather shockingly, this commit does not remove any static buffers, but I hope you will forgive it, because it does remove quite a bit of code. The final two commits I have queued up do remove static buffers.)

[kapfab]

Retried filename completion with directories, subdirectories and files, with and without quotes and spaces, seems to work fine.

[rrthomas]

Sorry, I should have been more specific. The glob-matching code I just replaced does not apply to completion, but to the wildcard patterns used for mget and mpush.

[kapfab]

Arf, my mistake! I'll test the right thing as soon as I can!

[kapfab]

Just tried a matching and non-matching f*-like pattern and they work, so I guess other types of patterns can only work.

[rrthomas]

Great stuff! The penultimate commit I have reimplements the yes/no prompts in mput and mget using gnulib's yesno module. As a side-effect, this accepts any "non-yes" response as "no", similarly to rm -i and mv -i, rather than looping until it gets a definite yes or no.

So, this will again require testing mget and mput.

[kapfab]

All works fine with yesno!

[rrthomas]

Many thanks! The last commit I have rewrites getCommand to use a vector<char *> rather than a static buffer to hold the tokens of the command. Thus, testing one or two commands with, say, 0, 1 and 2 tokens in them should suffice.

[kapfab]

Commands work fine but I get a segfault if I just press enter without typing any command. It’s a stroke of luck I mistyped ? otherwise I would have missed this bug.