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

[Albert Astals Cid]

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
?

> 
> 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?

Albert

> 
> NOTE: The "simple" html case is not outputting surrounding <text> or
> <div> tags, so does not have this issue; it is only the "complex"
> cases.
> 
> - Reece
>