Update 3rd party libs, part1

[georgeliao]

link to all multipass related forked repos https://github.com/orgs/canonical/teams/multipass/repositories
This is only part1, part2 is #3707

Private part, https://github.com/canonical/multipass-private/pull/666

FetchContent:

1. googletest, main branch as GIT_TAG, nothing to update

git submodule:

• Original repo
  1. fmt, 10.1.1 -> 11.0.2, includes small code adjustment.
  2. xz-decode, linux-2.6.38-26-g090e6a0 -> v20240322.
  3. scope_guard, nothing to update
  4. flutter, current version 3.16.9. Its update is out of the scope of this PR.
  5. protobuf.dart, current version protobuf-v1.1.4-251-gc559fe. Its update is out of the scope of this PR.
• Forked repo, aka with our patches as top commits.
  1. libssh currently is on 0.10.5 it is updated to 0.11.1, forked repo
  2. semver, current version is the latest version 1.1.0
  3. yaml-cpp, updated from 0.6.3 to 0.8.0, forked repo

vcpkg:

1. Poco, 1.12.4 -> 1.13.3

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.92%. Comparing base (76172f4) to head (d79d2c9).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3616      +/-   ##
==========================================
+ Coverage   88.88%   88.92%   +0.03%     
==========================================
  Files         254      254              
  Lines       14293    14294       +1     
==========================================
+ Hits        12705    12711       +6     
+ Misses       1588     1583       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Sploder12]

LGTM so far, just a couple links that should be updated. Notify me when gRPC is ready! :)

[georgeliao]

This change is due to the regex match failure stems from the change in the internal CMakeLists.txt change of libssh, the line
project(libssh VERSION 0.11.1 LANGUAGES C CXX) is appended with CXX.

[georgeliao]

Using multipass branch name as opposed to commit hash to prevent adaptive change in the future.

[georgeliao]

Hi @Sploder12
The part1 of 3rd party library update is ready for review.

I did a final scan on the latest version of the packages, it turned out that libssh can be upgraded further to 0.11.1 instead of 0.10.6. So the corresponding cherry-pick and push were done. Please perform a careful check on the new patches, meaning compare the patches with the ones on the 0.10.5 and see if it makes sense. You can find it either via submodule_info.md patch link or jump to the libssh repo by yourself. Please do the very same thing to yaml-cpp as well, if you have done that already, just simply let me know it looks good to you.

[Sploder12]

Mostly good. Tests pass and didn't notice any issues when running! Just some issues with edge cases in libssh changes.

[georgeliao]

line 708: potential off by one error, the check should probably be >=

Not sure I get this one, did you mean val is 0-index based so val == sftp->total_allocated_handles is already an invalid condition?

    memcpy(&val, ssh_string_data(handle), sizeof(uint32_t));

    if (val > sftp->total_allocated_handles) {
      return NULL;
    }

[georgeliao]

in file src/sftpserver.c

lines 674-676: if realloc fails then sftp->total_allocated_handles is still as if the realloc succeeded, this could lead to out-of-bounds issues later on in the program.

  if (i == sftp->total_allocated_handles) {
    uint32_t old_size = sftp->total_allocated_handles;
    sftp->total_allocated_handles += SFTP_HANDLES_CHUNK_SZ;

    void **tmp = realloc(sftp->handles, sftp->total_allocated_handles * sizeof(void *));
    if (tmp == NULL)
        return NULL; /* no handle available */

    sftp->handles = tmp;
    memset(sftp->handles + old_size, '\0', SFTP_HANDLES_CHUNK_SZ * sizeof(void *));
  }

Indeed, sftp->total_allocated_handles should be incremented only if realloc succeeds, what do you think of the proposed code snippet below?

  if (i == sftp->total_allocated_handles) {
    uint32_t temp_new_size = sftp->total_allocated_handles + SFTP_HANDLES_CHUNK_SZ;

    void **tmp = realloc(sftp->handles, temp_new_size * sizeof(void *));
    if (tmp == NULL)
        return NULL; /* no handle available */

    sftp->handles = tmp;
    uint32_t old_size = sftp->total_allocated_handles;
    sftp->total_allocated_handles = temp_new_size;
    memset(sftp->handles + old_size, '\0', SFTP_HANDLES_CHUNK_SZ * sizeof(void *));
  }

[Sploder12]

line 708: potential off by one error, the check should probably be >=

Not sure I get this one, did you mean val is 0-index based so val == sftp->total_allocated_handles is already an invalid condition?

    memcpy(&val, ssh_string_data(handle), sizeof(uint32_t));

    if (val > sftp->total_allocated_handles) {
      return NULL;
    }

Yeah, if val == sftp->total_allocated_handles then there is an out of bounds read afterwards.

[Sploder12]

  if (i == sftp->total_allocated_handles) {
    uint32_t old_size = sftp->total_allocated_handles;
    sftp->total_allocated_handles += SFTP_HANDLES_CHUNK_SZ;

    void **tmp = realloc(sftp->handles, sftp->total_allocated_handles * sizeof(void *));
    if (tmp == NULL)
        return NULL; /* no handle available */

    sftp->handles = tmp;
    memset(sftp->handles + old_size, '\0', SFTP_HANDLES_CHUNK_SZ * sizeof(void *));
  }

Indeed, sftp->total_allocated_handles should be incremented only if realloc succeeds, what do you think of the proposed code snippet below?

  if (i == sftp->total_allocated_handles) {
    uint32_t temp_new_size = sftp->total_allocated_handles + SFTP_HANDLES_CHUNK_SZ;

    void **tmp = realloc(sftp->handles, temp_new_size * sizeof(void *));
    if (tmp == NULL)
        return NULL; /* no handle available */

    sftp->handles = tmp;
    uint32_t old_size = sftp->total_allocated_handles;
    sftp->total_allocated_handles = temp_new_size;
    memset(sftp->handles + old_size, '\0', SFTP_HANDLES_CHUNK_SZ * sizeof(void *));
  }

The proposed code looks good!

[georgeliao]

@Sploder12
Another thing is that the windows build has been failing on ssh lib compilation since we upgraded to 0.10.6 from 0.10.5, the private part has a small change to fix that. See the intro comment for the link to the private side.

[georgeliao]

Yeah, if val == sftp->total_allocated_handles then there is an out of bounds read afterwards.

Then we are also fixing the libssh original code as opposed to fixing out patches. Not sure if we should do that.

The proposed code looks good!

This one is fixing the legacy patches as opposed to this particular rebase. So maybe this can be done.

[georgeliao]

@Sploder12

The proposed code looks good!

This is added to libssh repo. Please have another check. The other change will deviate from the original code of libssh and it adds complexity to the patches even though it might be a corner case bug. So I am inclined to leave it. What do you think?

[Sploder12]

This is added to libssh repo. Please have another check. The other change will deviate from the original code of libssh and it adds complexity to the patches even though it might be a corner case bug. So I am inclined to leave it. What do you think?

Looks good! It's not a big bug, and I doubt it's possible under normal execution, so I think leaving it is fine. Sorry, GitHub doesn't really make submodule changes clear.

[georgeliao]

@Sploder12
The latest commit cherry-pick was redone, so now it only grafts the commit. Feel free to make another PR to adapt the format and add the if block code we have proposed in a different PR.

Once the build is finished, feel free to merge.

[Sploder12]

LGTM!

[ricab]

This won't pass on the merge queue without the private side, need to merge manually.