diff --git a/Android.mk b/Android.mk index 06d98eb..8a1ffa0 100644 --- a/Android.mk +++ b/Android.mk @@ -69,6 +69,11 @@ common_static_libraries := \ libbatteryservice \ libavb \ +# TODO: include "cert-err34-c" once we move to Binder +# TODO: include "cert-err58-cpp" once 36656327 is fixed +common_local_tidy_flags := -warnings-as-errors=clang-analyzer-security*,cert-* +common_local_tidy_checks := -*,clang-analyzer-security*,cert-*,-cert-err34-c,-cert-err58-cpp + vold_conlyflags := -std=c11 vold_cflags := -Werror -Wall -Wno-missing-field-initializers -Wno-unused-variable -Wno-unused-parameter @@ -87,6 +92,9 @@ include $(CLEAR_VARS) LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk LOCAL_MODULE := libvold LOCAL_CLANG := true +LOCAL_TIDY := true +LOCAL_TIDY_FLAGS := $(common_local_tidy_flags) +LOCAL_TIDY_CHECKS := $(common_local_tidy_checks) LOCAL_SRC_FILES := $(common_src_files) LOCAL_C_INCLUDES := $(common_c_includes) LOCAL_SHARED_LIBRARIES := $(common_shared_libraries) @@ -103,6 +111,9 @@ include $(CLEAR_VARS) LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk LOCAL_MODULE := vold LOCAL_CLANG := true +LOCAL_TIDY := true +LOCAL_TIDY_FLAGS := $(common_local_tidy_flags) +LOCAL_TIDY_CHECKS := $(common_local_tidy_checks) LOCAL_SRC_FILES := \ main.cpp \ $(common_src_files) @@ -123,6 +134,9 @@ include $(CLEAR_VARS) LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk LOCAL_CLANG := true +LOCAL_TIDY := true +LOCAL_TIDY_FLAGS := $(common_local_tidy_flags) +LOCAL_TIDY_CHECKS := $(common_local_tidy_checks) LOCAL_SRC_FILES := vdc.cpp LOCAL_MODULE := vdc LOCAL_SHARED_LIBRARIES := libcutils libbase @@ -136,6 +150,9 @@ include $(CLEAR_VARS) LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk LOCAL_CLANG := true +LOCAL_TIDY := true +LOCAL_TIDY_FLAGS := $(common_local_tidy_flags) +LOCAL_TIDY_CHECKS := $(common_local_tidy_checks) LOCAL_SRC_FILES:= secdiscard.cpp LOCAL_MODULE:= secdiscard LOCAL_SHARED_LIBRARIES := libbase diff --git a/CommandListener.cpp b/CommandListener.cpp index c2b8310..78973db 100644 --- a/CommandListener.cpp +++ b/CommandListener.cpp @@ -327,8 +327,8 @@ int CommandListener::StorageCmd::runCommand(SocketClient *cli, continue; } - char processName[255]; - Process::getProcessName(pid, processName, sizeof(processName)); + std::string processName; + Process::getProcessName(pid, processName); if (Process::checkFileDescriptorSymLinks(pid, argv[2]) || Process::checkFileMaps(pid, argv[2]) || @@ -337,7 +337,7 @@ int CommandListener::StorageCmd::runCommand(SocketClient *cli, Process::checkSymLink(pid, argv[2], "exe")) { char msg[1024]; - snprintf(msg, sizeof(msg), "%d %s", pid, processName); + snprintf(msg, sizeof(msg), "%d %s", pid, processName.c_str()); cli->sendMsg(ResponseCode::StorageUsersListResult, msg, false); } } diff --git a/Devmapper.cpp b/Devmapper.cpp index e56a4da..4b6942d 100644 --- a/Devmapper.cpp +++ b/Devmapper.cpp @@ -195,7 +195,7 @@ int Devmapper::create(const char *name, const char *loopFile, const char *key, char *geoParams = buffer + sizeof(struct dm_ioctl); // bps=512 spc=8 res=32 nft=2 sec=8190 mid=0xf0 spt=63 hds=64 hid=0 bspf=8 rdcl=2 infs=1 bkbs=2 - strcpy(geoParams, "0 64 63 0"); + strlcpy(geoParams, "0 64 63 0", DEVMAPPER_BUFFER_SIZE - sizeof(struct dm_ioctl)); geoParams += strlen(geoParams) + 1; geoParams = (char *) _align(geoParams, 8); if (ioctl(fd, DM_DEV_SET_GEOMETRY, io)) { diff --git a/Process.cpp b/Process.cpp index fd757d5..1c0f504 100644 --- a/Process.cpp +++ b/Process.cpp @@ -28,10 +28,17 @@ #include #define LOG_TAG "ProcessKiller" + +#include +#include +#include #include #include "Process.h" +using android::base::ReadFileToString; +using android::base::StringPrintf; + int Process::readSymLink(const char *path, char *link, size_t max) { struct stat s; int length; @@ -40,10 +47,10 @@ int Process::readSymLink(const char *path, char *link, size_t max) { return 0; if ((s.st_mode & S_IFMT) != S_IFLNK) return 0; - - // we have a symlink + + // we have a symlink length = readlink(path, link, max- 1); - if (length <= 0) + if (length <= 0) return 0; link[length] = 0; return 1; @@ -63,16 +70,9 @@ int Process::pathMatchesMountPoint(const char* path, const char* mountPoint) { return 0; } -void Process::getProcessName(int pid, char *buffer, size_t max) { - int fd; - snprintf(buffer, max, "/proc/%d/cmdline", pid); - fd = open(buffer, O_RDONLY | O_CLOEXEC); - if (fd < 0) { - strcpy(buffer, "???"); - } else { - int length = read(fd, buffer, max - 1); - buffer[length] = 0; - close(fd); +void Process::getProcessName(int pid, std::string& out_name) { + if (!ReadFileToString(StringPrintf("/proc/%d/cmdline", pid), &out_name)) { + out_name = "???"; } } @@ -103,7 +103,7 @@ int Process::checkFileDescriptorSymLinks(int pid, const char *mountPoint, char * // append the file name, after truncating to parent directory path[parent_length] = 0; - strcat(path, de->d_name); + strlcat(path, de->d_name, PATH_MAX); char link[PATH_MAX]; @@ -189,24 +189,24 @@ int Process::killProcessesWithOpenFiles(const char *path, int signal) { while ((de = readdir(dir))) { int pid = getPid(de->d_name); - char name[PATH_MAX]; - if (pid == -1) continue; - getProcessName(pid, name, sizeof(name)); + + std::string name; + getProcessName(pid, name); char openfile[PATH_MAX]; if (checkFileDescriptorSymLinks(pid, path, openfile, sizeof(openfile))) { - SLOGE("Process %s (%d) has open file %s", name, pid, openfile); + SLOGE("Process %s (%d) has open file %s", name.c_str(), pid, openfile); } else if (checkFileMaps(pid, path, openfile, sizeof(openfile))) { - SLOGE("Process %s (%d) has open filemap for %s", name, pid, openfile); + SLOGE("Process %s (%d) has open filemap for %s", name.c_str(), pid, openfile); } else if (checkSymLink(pid, path, "cwd")) { - SLOGE("Process %s (%d) has cwd within %s", name, pid, path); + SLOGE("Process %s (%d) has cwd within %s", name.c_str(), pid, path); } else if (checkSymLink(pid, path, "root")) { - SLOGE("Process %s (%d) has chroot within %s", name, pid, path); + SLOGE("Process %s (%d) has chroot within %s", name.c_str(), pid, path); } else if (checkSymLink(pid, path, "exe")) { - SLOGE("Process %s (%d) has executable path within %s", name, pid, path); + SLOGE("Process %s (%d) has executable path within %s", name.c_str(), pid, path); } else { continue; } diff --git a/Process.h b/Process.h index 62a9313..4ddbdb9 100644 --- a/Process.h +++ b/Process.h @@ -28,7 +28,7 @@ public: static int checkFileMaps(int pid, const char *path, char *openFilename, size_t max); static int checkFileDescriptorSymLinks(int pid, const char *mountPoint); static int checkFileDescriptorSymLinks(int pid, const char *mountPoint, char *openFilename, size_t max); - static void getProcessName(int pid, char *buffer, size_t max); + static void getProcessName(int pid, std::string& out_name); private: static int readSymLink(const char *path, char *link, size_t max); static int pathMatchesMountPoint(const char *path, const char *mountPoint); diff --git a/Utils.cpp b/Utils.cpp index 72d3801..8bdae92 100644 --- a/Utils.cpp +++ b/Utils.cpp @@ -292,7 +292,7 @@ status_t ForkExecvp(const std::vector& args, LOG(ERROR) << "Failed to setexeccon"; abort(); } - FILE* fp = popen(cmd.c_str(), "r"); + FILE* fp = popen(cmd.c_str(), "r"); // NOLINT if (setexeccon(nullptr)) { LOG(ERROR) << "Failed to setexeccon"; abort(); diff --git a/VolumeManager.cpp b/VolumeManager.cpp index 3b4c054..e038303 100644 --- a/VolumeManager.cpp +++ b/VolumeManager.cpp @@ -180,7 +180,7 @@ static int setupDevMapperDevice(char* buffer, size_t len, const char* loopDevice } *createdDMDevice = true; } else { - strcpy(buffer, loopDevice); + strlcpy(buffer, loopDevice, len); *createdDMDevice = false; } return 0; @@ -931,7 +931,7 @@ int VolumeManager::createAsec(const char *id, unsigned long numSectors, const ch cleanupDm = true; } else { sb.c_cipher = ASEC_SB_C_CIPHER_NONE; - strcpy(dmDevice, loopDevice); + strlcpy(dmDevice, loopDevice, sizeof(dmDevice)); } /* @@ -1895,7 +1895,7 @@ int VolumeManager::listMountedObbs(SocketClient* cli) { // Create a string to compare against that has a trailing slash int loopDirLen = strlen(VolumeManager::LOOPDIR); char loopDir[loopDirLen + 2]; - strcpy(loopDir, VolumeManager::LOOPDIR); + strlcpy(loopDir, VolumeManager::LOOPDIR, sizeof(loopDir)); loopDir[loopDirLen++] = '/'; loopDir[loopDirLen] = '\0'; diff --git a/cryptfs.cpp b/cryptfs.cpp index f2f0f18..5d1453f 100644 --- a/cryptfs.cpp +++ b/cryptfs.cpp @@ -1725,7 +1725,8 @@ int cryptfs_setup_ext_volume(const char* label, const char* real_blkdev, memset(&ext_crypt_ftr, 0, sizeof(ext_crypt_ftr)); ext_crypt_ftr.fs_size = nr_sec; ext_crypt_ftr.keysize = keysize; - strcpy((char*) ext_crypt_ftr.crypto_type_name, "aes-cbc-essiv:sha256"); + strlcpy((char*) ext_crypt_ftr.crypto_type_name, "aes-cbc-essiv:sha256", + MAX_CRYPTO_TYPE_NAME_LEN); return create_crypto_blk_dev(&ext_crypt_ftr, key, real_blkdev, out_crypto_blkdev, label); @@ -2238,7 +2239,7 @@ static int cryptfs_enable_inplace_ext4(char *crypto_blkdev, } } - if (setjmp(setjmp_env)) { + if (setjmp(setjmp_env)) { // NOLINT SLOGE("Reading ext4 extent caused an exception\n"); rc = -1; goto errout;