[pacman-dev] [PATCH] Fix out of boundary reads in pacsort.

[Andrew Gregory]

On 06/11/16 at 12:14am, Tobias Stoeckmann wrote:
> On Thu, Jun 09, 2016 at 07:46:00PM -0400, Andrew Gregory wrote:
> > -t and -k are used to sort tabular data, where NUL is a perfectly sane
> > field separator.  In fact, with --machinereadable pacman will output
> > a table of NUL separated fields.  Though, pacsort does need its
> > handling of -t '\0' fixed in other places as well.
> 
> The function escape_char is only called from main for the 't' option
> argument. The result is stored in opts.delim.
> 
> The value of opts.delim is only used in function nth_column. That
> function is called in compare_versions, which is called by vercmp
> either directly or through compare_files.
> 
> And that one is called by main again. Which "other places" do you mean
> that have to be fixed? There is only one.
> 
> 
> The parsing of input happens in input_new, which allocates space for
> in->data by using strndup(). Whatever points into this new data area,
> accessing past the first '\0' is a straight out of boundary access.
> No matter the origin of pacsort's input, first '\0' definitely means
> 'end here'.
> 
> Maybe you mixed up the stand alone utility "pacsort" with some
> function in libalpm?
> 
> Otherwise please let me know what exactly is broken now by showing the
> code path or actual program invocation.

I was saying that your rationale for just dropping support for -t '\0'
was incorrect.  NUL not being legal in a path name is irrelevant.
But, as you just pointed out, pacsort has more issues with -t '\0'
than just the poor use of strchr referenced by your patch.  Hence,
"pacsort does need its handling of -t '\0' fixed in other places as
well."  In other words, if -t '\0' is kept, pacsort needs to be fixed
in a few places.

apg