查看原文
其他

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

The following article is from 阿里云开发者 Author 进之


导读


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

项目背景

我所在的项目组主要负责对店铺招牌拍摄,我负责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锁)竞争激烈,某一瞬间发生了数据冲突。

排查过程

首先熟悉了一下项目代码,下面是整个存储过程的流程图:
整个流程还是比较简单易懂的,按照我当时的怀疑方向,制定了以下排查顺序:
  1. 摄像头生成webp图片时出错了。
  2. 代码调用逻辑出错。
  3. 加密算法本身就有问题。

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

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

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

调用AES加密算法的时候,调用可能会出错。比如:由于偶然情况,同一个图片被连续调用了两次加密算法。要排查此问题,需要深入阅读此部分的代码,并进行梳理。
先查阅了AES加密算法的相关资料。
AES是高级加密标准,在密码学中又称Rijndael加密法,是美国联邦政府采用的一种区块加密标准。这个标准用来替代原先的DES,目前已经被全世界广泛使用,同时AES已经成为对称密钥加密中最流行的算法之一。AES支持三种长度的密钥:128位,192位,256位。

自己总结了一下:
  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的方式显示在屏幕上。上面这段代码的流程如下:
这里的逻辑很不合理,先把磁盘文件读取到内存解密,解密后再写回磁盘,此时磁盘上的图片是一个解密后的数据,再交由图片加载框架加载此图片,加载完成之后再进行加密,加密完再次写回文件。
此过程需要多次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方法,居然有同样的代码,相同的两行已经被注释掉了,看来之前的人也发现了此问题,但没有把两个地方的相同问题一并解决了。
代码改动之后,又打包进行测试,发现再也没有出现此问题了。最后能成功解决此问题,也是非常开心。

总结

从这件事情中,完美验证了“一个问题往往是由多个小的不规范或错误累积而成的”。每次的代码修改者,站在自身角度来看,没有造成大问题,单独来看,也不是完全不合理。
如果代码写得不规范,留有安全隐患,当时虽然不会暴露,所有风险问题汇总到一起的时候,就造成最后的“灾难”。
我也收获到了以下经验:
  1. 对于入参校验,应该提早进行校验,出现“非法入参”时,应当有合理的措施。比如:以返回值或者标志位的方式告诉调用者,实在不行可以造成崩溃,这样就能及早暴露问题。

  2. 底层模块要有通用性,不能只考虑当时的情况,此模块日后可能会在多种情况下使用;

  3. 要有风险意识,不要把风险问题扩大化,对同一份文件多次解密再加密,会出现错上加错的情况。

  4. 解决一个错误时,要看一下有没有相似的错误,可以一并修改。
继续滑动看下一个
OSC开源社区
向上滑动看下一个

您可能也对以下帖子感兴趣

文章有问题?点此查看未经处理的缓存