New issue
Advanced search Search tips

Issue 784031 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

protobuf transmission bug in imageloader

Project Member Reported by xiaochu@chromium.org, Nov 11 2017

Issue description

Currently in sender side, a Message is serialized to std::string and convert to a char array (size is equal to string size + 1). Then it is referred to a iovec inside a msghdr. 

In receiver side, the iovec is extracted to a 4*4096 char array. Then this char array feed into ParseFromString to generate the original Message object.

This works when the original string contains no '\0' between [0:size()-1]. However, proto2 may encode the serialized string with '\0' in the middle. In this case (I modified ipc.proto to support unmount and this happens, hard to debug...), the length parameter fed into ParseFromArray in the receiver side is shorter and thus the string to be deserialized is cut shorter. 

solution:
use msg_control to contain the message size instead of fd. Then move fd to protobuf message.

alternative solution:
use first byte to carry the message size. but I don't see any reason why we use msg_control to transmit fd since it's essentially a message instead of transmission control message.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/imageloader/+/1c3fbed9123c081c73b1bc97f755ccf7a753b74d

commit 1c3fbed9123c081c73b1bc97f755ccf7a753b74d
Author: Xiaochu Liu <xiaochu@chromium.org>
Date: Sat Nov 18 04:18:47 2017

Support umount in helper_process

Currently mount points cleanup is called directly in imageloader_main
without sandboxing. In order for cleanup to work in non-root user in
sandboxed environment (dbus call), I provide a method in helper_process
to perform umount in sandbox as root.

It also fixes a bug in message deserialization where message could be
cut shorter accidentally due to encoding with string delimeter in the
middle.

BUG= chromium:784031 , chromium:782334 
TEST=unittest, mount/umount images on DuT

Change-Id: Ib4c7522c96165c6ffae4cb0342137f8a0bdcc07b
Reviewed-on: https://chromium-review.googlesource.com/767024
Commit-Ready: Xiaochu Liu <xiaochu@chromium.org>
Tested-by: Xiaochu Liu <xiaochu@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>

[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/mount_helper.h
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/helper_process.cc
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/helper_process.h
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/mount_helper.cc
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/seccomp/imageloader-helper-seccomp-amd64.policy
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/ipc.proto
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/imageloader_impl.h
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/imageloader_main.cc
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/imageloader_impl.cc
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/seccomp/imageloader-helper-seccomp-arm.policy
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/mock_helper_process.h
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/imageloader_unittest.cc
[modify] https://crrev.com/1c3fbed9123c081c73b1bc97f755ccf7a753b74d/seccomp/imageloader-helper-seccomp-x86.policy

Status: Fixed (was: Untriaged)

Comment 3 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 4 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment