[poppler] RFC: Potential invalid pointer / release after free fix in HtmlOutputDev

[Reece Dunn]

2009/11/4 Albert Astals Cid <aacid at kde.org>:
> A Dimecres, 4 de novembre de 2009, Reece Dunn va escriure:
>> NOTE: I haven't built this, as I cannot build at the moment (missing
>> fontconfig dependency in Ubuntu that I haven't figured out).
>
> sudo apt-get install libfontconfig1-dev
> ?

That works, thanks.

>> The original code is something like:
>>
>>    GooString *str, *str1 = NULL;
>>    for(HtmlString *tmp=yxStrings;tmp;tmp=tmp->yxNext){
>>     if (tmp->htext){
>>        str=new GooString(tmp->htext);
>>        fprintf(f,"<text top=\"%d\" left=\"%d\"
>> ",xoutRound(tmp->yMin),xoutRound(tmp->xMin));
>>        fprintf(f,"width=\"%d\" height=\"%d\"
>> ",xoutRound(tmp->xMax-tmp->xMin),xoutRound(tmp->yMax-tmp->yMin));
>>        fprintf(f,"font=\"%d\">", tmp->fontpos);
>>       if (tmp->fontpos!=-1){
>>       str1=fonts->getCSStyle(tmp->fontpos, str);
>>       }
>>        fputs(str1->getCString(),f);
>>       delete str;
>>        delete str1;
>>        fputs("</text>\n",f);
>>      }
>>
>> As can be seen, str1 is not initialised outside of the "if
>> (tmp->fontpos!=-1)" condition. This means that if tmp->fontpos == -1,
>> str1 is either NULL (if this is the first entry, resulting in a NULL
>> pointer dereference), or is pointing to the previous str1 object
>> already deleted (resulting in a delete-after-free), and before it a
>> NULL pointer access, or access to already released memory (with
>> potentially garbage data).
>>
>> Thus, the code should be:
>>
>>    GooString *str, *str1 = NULL;
>>    for(HtmlString *tmp=yxStrings;tmp;tmp=tmp->yxNext){
>>     if (tmp->htext){
>>        str=new GooString(tmp->htext);
>>        fprintf(f,"<text top=\"%d\" left=\"%d\"
>> ",xoutRound(tmp->yMin),xoutRound(tmp->xMin));
>>        fprintf(f,"width=\"%d\" height=\"%d\"
>> ",xoutRound(tmp->xMax-tmp->xMin),xoutRound(tmp->yMax-tmp->yMin));
>>        fprintf(f,"font=\"%d\">", tmp->fontpos);
>>       if (tmp->fontpos!=-1){
>>       str1=fonts->getCSStyle(tmp->fontpos, str);
>>        fputs(str1->getCString(),f);
>>        delete str1;
>>       }
>>       delete str;
>>        fputs("</text>\n",f);
>>      }
>>
>> with str1 only being used and released inside that conditional.
>>
>> Now, if tmp->fontpos == -1 the output is "<text></text>" as there is
>> no text being written. This does not make sense, so the
>> (tmp->fontpos!=-1) check can be moved to the (tmp->htext) check.
>>
>> Thus my patch, which as I said is untested (hence the RFC).
>
> Yeah seems logical, but the more important, do you have a case where this
> happens?

No, I don't have a case where this happens. It is something I saw when
looking at the code.

That is, I don't know when tmp->fontpos == -1 (for any tmp in
xyStrings) without further analysis.

- Reece