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

原创进之阿里云开发者

阿里妹导读

一个问题往往是由多个小的不规范或错误累积而成的。本文记录了作者发现问题、现象分析、排查过程、最后解决问题的全历程。

项目背景

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

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

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

现象

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

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

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

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

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

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

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

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

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

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

排查过程

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

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

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

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

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

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

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

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

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

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

自己总结了一下：

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

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

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


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的方式显示在屏幕上。上面这段代码的流程如下：

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

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

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

修改为如下代码：

@Overrideprotected 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文件的标志位

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

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方法，居然有同样的代码，相同的两行已经被注释掉了，看来之前的人也发现了此问题，但没有把两个地方的相同问题一并解决了。

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

总结

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

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

我也收获到了以下经验：

对于入参校验，应该提早进行校验，出现“非法入参”时，应当有合理的措施。比如：以返回值或者标志位的方式告诉调用者，实在不行可以造成崩溃，这样就能及早暴露问题。
底层模块要有通用性，不能只考虑当时的情况，此模块日后可能会在多种情况下使用；
要有风险意识，不要把风险问题扩大化，对同一份文件多次解密再加密，会出现错上加错的情况。
解决一个错误时，要看一下有没有相似的错误，可以一并修改。

继续滑动看下一个