一个埋藏9年的底层bug发现历程

本文涉及的产品
密钥管理服务KMS,1000个密钥,100个凭据,1个月
简介: 项目背景:一个负责店铺招牌拍摄的项目,App客户端开发中遇到照片损坏问题,尤其是使用webp格式时。问题已存在一段时间,最初采取临时措施让用户删除损坏照片,服务端也做损坏检测。排查过程包括:确认摄像头和图片压缩无误,发现加密解密流程不合理,修复了多余加密操作,但问题依旧存在。进一步分析发现,解密算法在处理某些特定图片时可能出现错误,原因是算法在处理非字符串数据时,错误地将空字符视为字符串结束标志。最终,修复了这个错误,成功解决了照片损坏问题。

项目背景

我所在的项目组主要负责对店铺招牌拍摄,我负责App客户端的开发工作。此项目从立项之初到现在已经有很长的历史了。

现在出现了一个问题:用户在拍摄照片时,会出现照片损坏的情况,这个问题在线上环境出现了有一段时间了,再加上自己接手时,此问题已经出现了,就没有深入排查过产生原因。暂时的解决策略是让用户手动删除损坏的照片,上传图片时,服务端也会进行一次文件损坏检测。

我们会下发各种拍摄任务类型,有的任务只需要拍摄几张照片即可,有的任务需要拍摄上千张图片,此问题就会更容易暴露。在同事的建议下,决定要找到问题的根源。

现象

之前只是知道有此问题,没有仔细研究过。经过自测+了解,初步明确了以下现象:

现象1:不同任务类型都有此问题

目前项目内的不同任务类型都共用同一个拍照存储模块。此现象可以明确,出错范围是在底层拍照存储模块,而不是在上层的业务逻辑。

现象2:1/200的概率稳定出现图片损坏

通过与同事的共同复现,发现连续拍摄200多张的时候,就会出现一张损坏的图片。这中间我们复现好多次,出现频率都很符合预期,甚至有一丝诡异,因为这个bug出现频率太稳定了,反而有些不正常了。面对此现象,当时想到了2种可能的情况:

  1. 概率和1/256(16进制的FF转为十进制的值,2的8次方,一字节[Byte]的大小)很接近,是不是由于在解析到某一字节时,出现了异常。
  2. 每拍摄200多张,此时就出现 重GC+手机温度过高导致降频,导致了卡顿,造成某一步执行超时或者失败。

以上只是猜测,完全没有任何证据,只是当时的思考方向。

现象3:仅webp格式会出现此问题

目前拍摄的图片有两种存储格式,分别是jpeg和webp格式。项目之前都是使用JPEG作为存储格式,后来为了减小图片的大小,开始改用webp格式进行存储。当我们把存储格式改为jpeg时,此问题不会出现;换为webp格式时,就是出现此问题。

统计了这两者的整体耗时(从图片字节流到存储到文件中),webp的用时大概是jpeg耗时的5倍;jpeg的存储大小是webp大小的1.5倍左右。面对此现象,当时的想法是处理图片耗时久,因而导致锁(线程锁、IO锁)竞争激烈,某一瞬间发生了数据冲突。

排查过程

首先熟悉了一下项目代码,下面是整个存储过程的流程图:

image.png


整个流程还是比较简单易懂的,按照我当时的怀疑方向,制定了以下排查顺序:

  1. 摄像头生成webp图片时出错了。
  2. 代码调用逻辑出错。
  3. 加密算法本身就有问题。

排查方向1:压制照片时出错

摄像头输出的图片在压制为webp照片的时候,就出现损坏了,而jpeg压制时不会损坏。该问题排查比较简单,只需要把未加密的原始webp图片也存储下来,与加密后无法解密的图片进行对照即可。实践之后,发现损坏的加密图片,对应的原始webp照片都是可以正常展示的

因此可以明确排除手机摄像头和压制webp图片的问题。

排查方向2:加密流程产生问题

调用AES加密算法的时候,调用可能会出错。比如:由于偶然情况,同一个图片被连续调用了两次加密算法。要排查此问题,需要深入阅读此部分的代码,并进行梳理。

先查阅了AES加密算法的相关资料。

AES是高级加密标准,在密码学中又称Rijndael加密法,是美国联邦政府采用的一种区块加密标准。这个标准用来替代原先的DES,目前已经被全世界广泛使用,同时AES已经成为对称密钥加密中最流行的算法之一。AES支持三种长度的密钥:128位,192位,256位。

image.png

自己总结了一下:

  1. AES算法属于对称加密,加密和解密只需要一个相同的密钥;
  2. AES算法在对明文加密的时候,并不是把整个明文一股脑加密成一整段密文,而是把明文拆分成一个个独立的明文块,每一个明文块长度128bit;
  3. 在没有填充的情况下,密文和原文长度相等。

先重点看了一下线程安全问题,排查一圈,认真看了在此过程中所有涉及的共享变量,没有发现任何问题。

下面梳理了加密解密流程,发现了一个很严重的问题。此问题发生在预览图片部分,代码如下:

public Bitmap decode(ImageDecodingInfo decodingInfo) throws IOException {
    // 如果是本地图片,OriginalImageUri会是'file:///xxx'以此来判断是否正在加载本地图片
    boolean isLoaclFile = decodingInfo.getOriginalImageUri().startsWith("file://");
    String imagePath = null;
    if (isLoaclFile) {
        imagePath = decodingInfo.getImageUri().replaceFirst("file://", "");
        if (!TextUtils.isEmpty(imagePath) && new File(imagePath).exists()) {
            ImageEncryptTool.decrypt(imagePath);
        } else {
            return null;
        }
    }
    Bitmap bitmap = super.decode(decodingInfo);
    if (isLoaclFile) {
        ImageEncryptTool.encrypt(imagePath);
    }
    return bitmap;
}
// 解密代码
public static void encrypt(String filePath) throws IOException {
    try {
        RandomAccessFile raf = new RandomAccessFile(file, "rw");
        byte[] buffer = new byte[ENCRYPTED_SIZE];
        raf.read(buffer);
        buffer = JniArithmetic.aesEncryptNoPadding(buffer);
        raf.seek(0);
        raf.write(buffer);
        raf.close();
    } catch (IOException e) {
        e.printStackTrace();
        throw e;
    }
}

手机预览图片时,需要从手机磁盘中读取照片,进行解密后,转为bitmap的方式显示在屏幕上。上面这段代码的流程如下:

image.png

这里的逻辑很不合理,先把磁盘文件读取到内存解密,解密后再写回磁盘,此时磁盘上的图片是一个解密后的数据,再交由图片加载框架加载此图片,加载完成之后再进行加密,加密完再次写回文件。

此过程需要多次IO操作,执行效率很低。此过程不能保证是“原子性”操作,在流程中,发生任何问题都会导致最终的图片发生损坏,比如解密完成之后,由于崩溃导致没有进行加密。不合理的方式大大增加了出错概率。

另外,还有一个更严重的问题,当解密文件覆盖原文件后,另外一个线程可能会调用此文件,会把已经解密后的文件,再一次进行解密,解密完之后再重新覆盖写回,最终文件就是“一团乱麻”,会造成图片损坏。

修改为如下代码:

@Override
protected InputStream getImageStream(ImageDecodingInfo decodingInfo) throws IOException {
    // 如果是本地图片,OriginalImageUri会是'file:///xxx'以此来判断是否正在加载本地图片
    boolean isLoaclFile = decodingInfo.getOriginalImageUri().startsWith("file://");
    if (!isLoaclFile) {
        return super.getImageStream(decodingInfo);
    }
    // 解密本地图片
    InputStream imageStream = super.getImageStream(decodingInfo);
    byte[] encodeByteArray = inputStreamToByteArray(imageStream);
    final int ENCRYPTED_SIZE = 1024;
    byte[] decodeBuffer = new byte[ENCRYPTED_SIZE];
    System.arraycopy(encodeByteArray, 0, decodeBuffer, 0, ENCRYPTED_SIZE);
    decodeBuffer = JniArithmetic.aesDecryptNoPadding(decodeBuffer);
    System.arraycopy(decodeBuffer, 0, encodeByteArray, 0, ENCRYPTED_SIZE);
    return new ByteArrayInputStream(encodeByteArray);
}

在正确且合理的流程中,解密操作只会在内存中进行,不会写入到磁盘之中,这样就不会产生各种覆盖的情况了。

最后又排查了整个项目,移除了所有【解密再加密】 的过程,整个项目就保留了一处加密的地方,就是在第一次保存图片的时候,才会进行加密,然后再写入磁盘中。

成功解决?

这么明显的错误都被解决了,这时候想着,肯定没啥问题了。怀着十足的信心,进行了一番测试,可此时依然有此问题。刚开始有点不太确定,试了多次之后,可能100%确定问题依然未解决。

排查方向3:锁竞争问题

这时候又把视角转到了线程安全方向,为了使整个加密存储的耗时可控,我决定自己实现加解密算法。当然,我自己实现的加解密算法很简单。代码如下:

private static final int N=100;
public static byte[] aesEncrypt(byte[] data) {
    for (int i = 0; i < N; i++) {
        data[i] = (byte) (data[i] + 1);
    }
    return data;
}
public static byte[] aesDecrypt(byte[] data) {
    for (int i = 0; i < N; i++) {
        data[i] = (byte) (data[i] - 1);
    }
    return data;
}

只是简单地把每一个字节的值+1,解密的时候,再把每一个字节-1进行解密,我可以使用N的大小控制加解密耗时。又进行了一番测试,这时候不论怎么调整加解密耗时,都没有发生图片损坏的情况。

通过现有信息,加解密算法应该很有问题。但我依然相信加密算法没有问题,加密代码已经存在了9年之久,而且用的是很成熟的AES加密算法,应该不会有问题。

排查方向4:图片问题

从手机中把损坏的图片单独取出来,又分别用加密算法、解密算法处理这张图片。拿到了以下数据:

图1:未加密的原始照片,可以看到以RIFF开头,是用来识别webp文件的标志位

image.png

图2:按照代码流程加密结果

image.png

  • Case1:把原始图片,用加密算法单独跑一遍,发现内容和图2一致;
  • Case2:把加密图片,用解密算法单独运行一遍,发现内容和图1不一致,尝试多次后发现,每次解密的数据居然都是随机内容。

对应这一张图片,每次都把解密后的内容打印出来,发现有时候正确,有时候又是随机的,而且修改先后执行顺序,结果也可能不一样。由于加解密算法是由C++所写,根据以往经验,猜测出现此种情况,是由于C++内存残留导致的。

服务端在使用图片时,也需要进行解密,由于服务端不怕代码泄漏,因此直接使用了Java类库实现AES解密算法。在服务端同事的配合下,单独上传该图片,尝试了多次,发现服务端是可以稳定进行解密的。

又再服务端同事的帮助下,拿到了服务端解密算法,我把端上的解密算法替换为服务端解密算法,这张损坏的图片居然又可以正确展示了。最后又经过一番测试,发现再也没有出现图片损坏的情况。

到此,已经有95%的把握,可以证明是解密算法导致的。此时也可以安心下班了,第二天再排查解密算法。

排查方向5:AES解密算法

先向同事要到了加解密仓库的git地址,由于这块代码历史比较悠久,立项之初,使用SVN进行管理,后来迁移时整体被打包放到git仓库里,已经无法看到提交记录了。

项目本身的架构也比较老,使用了Android.mk作为构建工具,现在已经废弃很久了,我也没有接触过。在ChatGPT的帮助下,自动帮我转换成了现代的CMakeLists构建工具。接下来就可以正常的debug跟踪代码了。

AES算法本身就比较简单,只是不停地按照密码表,对原文进行替换。代码中没有使用任何三方库,自己实现了AES算法。

解密算法如下:

void AES::InvCipher( BYTE *input, BYTE *output, int len)
{
    int arrLen = len;
    unsigned char *uch_input = new unsigned char[arrLen + 1];
    strToUChar((const char*)input, uch_input, len);
    for (int i = 0; i < arrLen / 16; i++) {
        InvCipher(uch_input + i*16);
    }
    ucharToStr((const unsigned char*)uch_input, (char *)output, len);
    delete[] uch_input;
}
int AES::strToUChar(const char *ch, unsigned char *uch, int len) {
    int tmp = 0;
    if (ch == NULL || uch == NULL)
        return -1;
    if (strlen(ch) == 0)
        return -2;
    while (len) {
        tmp = (int) *ch;
        *uch++ = tmp;
        ch++;
        len--;
    }
    *uch = (int) '\0';
    return 0;
}

在跟踪到 if (strlen(ch) == 0) 这一行代码时,发现会直接返回-2作为错误码,上面的处理过程会直接忽视错误码,继续进行解密。当然,这时候肯定是无法解密成功的。

错误原因

我对这一做法进行了合理猜测,刚开始此处的加密只用于字符串,所以在入参时,会判断一下是否为空字符串。

在C++中,字符串通常以字符数组的形式表示,遵循C语言的传统。C/C++中的字符串是以空字符\0(ASCII值为0)结尾的字符数组。这种字符串被称为C-风格字符串或null-terminated字符串。判断字符串结束的方式就是检查每个字符,直到遇到\0。

char str[] = "hello";
// 字符串实际存储为 {'h', 'e', 'l', 'l', 'o', '\0'}

在JAVA中,字符串本身就存储了字符串的长度。这个长度字段在String对象创建时就被计算并存储起来,因此可以非常快速地调用length()方法来得到字符串的长度,而不需要遍历整个字符数组。

但是后来需要对二进制图片进行加密,二进制图片在存储过程中,也会用到'0x00'字节,但和字符串中的'0x00'含义完全不一样。如果二进制图片第一个字节为0x00,当成字符串处理就会得出加密内容为空,因而终止后续流程

又找了几张解密失败的图片,发现它们无一例外,开头第一个字节都是0x00,怪不得失败的概率是1/256。又把jpeg图片找来,所有jpeg图片的前16个字节(目测前500字节都是)是固定的,因此加密之后第一个字节就是固定。而webp格式加密后第一个字节是随机的,当然不会出现问题了。

问题解决

由于这部分代码已经存在了9年了,再加上自己对此部分代码不是很熟悉,秉承着最小改动的原则,只是去掉了对空字符串校验的情况,空字符可以提前校验。

int AES::strToUChar(const char *ch, unsigned char *uch, int len) {
    int tmp = 0;
    if (ch == NULL || uch == NULL)
        return -1;
//      if (strlen(ch) == 0)
//          return -2;
    while (len) {
        tmp = (int) *ch;
        *uch++ = tmp;
        ch++;
        len--;
    }
    *uch = (int) '\0';
    return 0;
}

后来在整理时,发现另外一个charToHex方法,居然有同样的代码,相同的两行已经被注释掉了,看来之前的人也发现了此问题,但没有把两个地方的相同问题一并解决了。

代码改动之后,又打包进行测试,发现再也没有出现此问题了。 最后能成功解决此问题,也是非常开心。

总结

从这件事情中,完美验证了“一个问题往往是由多个小的不规范或错误累积而成的”。每次的代码修改者,站在自身角度来看,没有造成大问题,单独来看,也不是完全不合理。

如果代码写得不规范,留有安全隐患,当时虽然不会暴露,所有风险问题汇总到一起的时候,就造成最后的“灾难”。

我也收获到了以下经验:

  1. 对于入参校验,应该提早进行校验,出现“非法入参”时,应当有合理的措施。比如:以返回值或者标志位的方式告诉调用者,实在不行可以造成崩溃,这样就能及早暴露问题。
  2. 底层模块要有通用性,不能只考虑当时的情况,此模块日后可能会在多种情况下使用;
  3. 要有风险意识,不要把风险问题扩大化,对同一份文件多次解密再加密,会出现错上加错的情况。
  4. 解决一个错误时,要看一下有没有相似的错误,可以一并修改。
目录
相关文章
|
3月前
|
存储 算法 Java
一个埋藏9年的底层bug发现历程
一个问题往往是由多个小的不规范或错误累积而成的。本文记录了作者发现问题、现象分析、排查过程、最后解决问题的全历程。
|
测试技术 数据库 安全
带你读《C++代码整洁之道:C++17 可持续软件开发模式实践》之二:构建安全体系
如果想用C++语言编写出易维护的、扩展性良好的以及生命力强的软件,那么,对于所有的软件开发人员、软件设计人员、对现代C++代码感兴趣或想降低开发成本的项目领导者来说,本书都是必需品。如果你想自学编写整洁的C++代码,那么本书也是你需要的。本书旨在通过一些示例帮助各个技术层次的开发人员编写出易懂的、灵活的、可维护的和高效的C++代码。即使你是一名资深的开发工程师,在本书中也可以找到有价值的知识点。
|
5月前
|
数据建模
技术经验解读:ZVS振荡电路工作原理分析
技术经验解读:ZVS振荡电路工作原理分析
102 1
|
6月前
|
算法 安全 数据安全/隐私保护
深入探究一个长期隐藏的底层bug的学习报告
在软件开发的过程中,底层bug往往像一颗定时炸弹,随时可能引发严重的问题。本文将分享我在开发过程中遇到的一个长期未被发现的底层bug,以及我如何逐步排查并最终解决这个问题的全过程。通过这次排查,我深刻认识到了代码规范性的重要性。一个不规范的代码修改,虽然短期内可能不会引起问题,但长期累积下来,可能会引发灾难性的后果。此外,我也意识到了底层模块的通用性和风险意识的重要性。在解决一个问题的同时,应该审视是否有相似的问题存在,以避免未来的风险。
128 3
|
6月前
|
存储 缓存 算法
作者推荐 | 【底层服务/编程功底系列】「底层技术原理」史上最清晰的采用程序员的视角方式进行深入探索Linux零拷贝技术原理及实现
作者推荐 | 【底层服务/编程功底系列】「底层技术原理」史上最清晰的采用程序员的视角方式进行深入探索Linux零拷贝技术原理及实现
59 0
|
6月前
|
NoSQL Java 程序员
阿里开发人员献礼“Java架构成长笔记”,深入内核,拒绝蒙圈
提起阿里,行外人联想到的关键词无非是“交易”、“淘宝”、“支付宝”,但对于程序员来说,阿里庞大的技术体系才是最吸引人的。实际上阿里作为国内一线互联网公司的头把交椅,内部的技术体系和发展都是备受关注的,对于程序员来说,能够进到阿里工作,就是对自己的技术水平进行一个提升和学习。
阿里开发人员献礼“Java架构成长笔记”,深入内核,拒绝蒙圈
|
程序员 开发者
|
SQL 安全 Java
硬核,腾讯内部整理的面向开发人员代码安全指南,适合所有程序员
硬核,腾讯内部整理的面向开发人员代码安全指南,适合所有程序员
115 0
|
测试技术 API
怎么在日常中提高你的编程找bug的能力
编程是一门需要细致入微和耐心的艺术。在编写代码的过程中,我们经常会遇到各种各样的错误和bug。因此,掌握一定的找bug能力对于提高代码质量和开发效率至关重要。本文将分享一些在日常中提高编程找bug能力的实用技巧。
146 0
|
开发框架 JSON 缓存
从三层架构说起,谈谈对历史项目的小改造
任何框架无论封装的如何优秀,关键还是在于局中 “玩游戏” 的开发者,框架只是提供了基本的开发规则,要大家共同的遵循,这其中最难的就是团队小伙伴达成一致的思维认知和共识。约定由于配置,任何框架不可能面面俱到,过渡设计框架务必会失去部分灵活性(物极必反的道理想必大家都知...
226 1
从三层架构说起,谈谈对历史项目的小改造
下一篇
无影云桌面