[1/3] normalize_path_copy(): document "dst" size expectations
diff mbox series

Message ID 20200130095219.GA840531@coredump.intra.peff.net
State New
Headers show
Series
  • some minor memory allocation cleanups
Related show

Commit Message

Jeff King Jan. 30, 2020, 9:52 a.m. UTC
We take a "dst" buffer to write into, but there's no matching "len"
parameter. The hidden assumption is that normalizing always makes things
smaller, so we're OK as long as "dst" is at least as big as "src". Let's
document that explicitly.

Signed-off-by: Jeff King <peff@peff.net>
---
 path.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Taylor Blau Jan. 30, 2020, 8:12 p.m. UTC | #1
On Thu, Jan 30, 2020 at 04:52:19AM -0500, Jeff King wrote:
> We take a "dst" buffer to write into, but there's no matching "len"
> parameter. The hidden assumption is that normalizing always makes things
> smaller, so we're OK as long as "dst" is at least as big as "src". Let's
> document that explicitly.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  path.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/path.c b/path.c
> index a76eec8b96..88cf593007 100644
> --- a/path.c
> +++ b/path.c
> @@ -1077,6 +1077,8 @@ const char *remove_leading_path(const char *in, const char *prefix)
>
>  /*
>   * It is okay if dst == src, but they should not overlap otherwise.
> + * The "dst" buffer must be at least as long as "src"; normalizing may shrink
> + * the size of the path, but will never grow it.

Thanks for documenting this. It's quite helpful, and hopefully should
prevent bugs like the one you alluded to in your cover letter from
getting in in the future.

>   *
>   * Performs the following normalizations on src, storing the result in dst:
>   * - Ensures that components are separated by '/' (Windows only)
> --
> 2.25.0.515.gaba5347bc6

This looks obviously good to me.

Thanks,
Taylor
Jeff King Jan. 31, 2020, 8:45 a.m. UTC | #2
On Thu, Jan 30, 2020 at 12:12:47PM -0800, Taylor Blau wrote:

> > @@ -1077,6 +1077,8 @@ const char *remove_leading_path(const char *in, const char *prefix)
> >
> >  /*
> >   * It is okay if dst == src, but they should not overlap otherwise.
> > + * The "dst" buffer must be at least as long as "src"; normalizing may shrink
> > + * the size of the path, but will never grow it.
> 
> Thanks for documenting this. It's quite helpful, and hopefully should
> prevent bugs like the one you alluded to in your cover letter from
> getting in in the future.

To be picky, I didn't find an actual bug around buffer lengths; the
problem was a failure to check the error code. This was just something I
happened to find confusing auditing the code.

-Peff

Patch
diff mbox series

diff --git a/path.c b/path.c
index a76eec8b96..88cf593007 100644
--- a/path.c
+++ b/path.c
@@ -1077,6 +1077,8 @@  const char *remove_leading_path(const char *in, const char *prefix)
 
 /*
  * It is okay if dst == src, but they should not overlap otherwise.
+ * The "dst" buffer must be at least as long as "src"; normalizing may shrink
+ * the size of the path, but will never grow it.
  *
  * Performs the following normalizations on src, storing the result in dst:
  * - Ensures that components are separated by '/' (Windows only)