| Message ID | 803590a35221fbf411b2c141419aea3233a6e990.1530294813.git.ernesto.mnd.fernandez@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2018-06-29 at 15:34 -0300, Ernesto A. Fernández wrote: > The hfs_find_exit() function expects fd->bnode to be NULL after a > search has failed. The hfs_brec_insert() function may instead set > it to an error-valued pointer. Fix this to prevent a crash. > > Reported-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com> > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > --- > fs/hfsplus/brec.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c > index 808f4d8c859c..ed8eacb34452 100644 > --- a/fs/hfsplus/brec.c > +++ b/fs/hfsplus/brec.c > @@ -73,9 +73,10 @@ int hfs_brec_insert(struct hfs_find_data *fd, void *entry, int entry_len) > if (!fd->bnode) { > if (!tree->root) > hfs_btree_inc_height(tree); > - fd->bnode = hfs_bnode_find(tree, tree->leaf_head); > - if (IS_ERR(fd->bnode)) > - return PTR_ERR(fd->bnode); Are you sure that no caller is used this error code? Did you check this? Maybe, it makes sense to extract the error code and to show the error message on the caller side instead of processing the simple NULL? Thanks, Vyacheslav Dubeyko. > + node = hfs_bnode_find(tree, tree->leaf_head); > + if (IS_ERR(node)) > + return PTR_ERR(node); > + fd->bnode = node; > fd->record = -1; > } > new_node = NULL;
On Mon, 02 Jul 2018 11:01:37 -0700 Viacheslav Dubeyko <slava@dubeyko.com> wrote: > On Fri, 2018-06-29 at 15:34 -0300, Ernesto A. Fernández wrote: > > The hfs_find_exit() function expects fd->bnode to be NULL after a > > search has failed. The hfs_brec_insert() function may instead set > > it to an error-valued pointer. Fix this to prevent a crash. > > > > Reported-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com> > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > > --- > > fs/hfsplus/brec.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c > > index 808f4d8c859c..ed8eacb34452 100644 > > --- a/fs/hfsplus/brec.c > > +++ b/fs/hfsplus/brec.c > > @@ -73,9 +73,10 @@ int hfs_brec_insert(struct hfs_find_data *fd, void *entry, int entry_len) > > if (!fd->bnode) { > > if (!tree->root) > > hfs_btree_inc_height(tree); > > - fd->bnode = hfs_bnode_find(tree, tree->leaf_head); > > - if (IS_ERR(fd->bnode)) > > - return PTR_ERR(fd->bnode); > > > Are you sure that no caller is used this error code? Did you check this? > > Maybe, it makes sense to extract the error code and to show the error > message on the caller side instead of processing the simple NULL? > No response? Could we please get this wrapped up?
On Tue, Aug 21, 2018 at 04:02:24PM -0700, Andrew Morton wrote: > On Mon, 02 Jul 2018 11:01:37 -0700 Viacheslav Dubeyko <slava@dubeyko.com> wrote: > > > On Fri, 2018-06-29 at 15:34 -0300, Ernesto A. Fernández wrote: > > > The hfs_find_exit() function expects fd->bnode to be NULL after a > > > search has failed. The hfs_brec_insert() function may instead set > > > it to an error-valued pointer. Fix this to prevent a crash. > > > > > > Reported-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com> > > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > > > --- > > > fs/hfsplus/brec.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c > > > index 808f4d8c859c..ed8eacb34452 100644 > > > --- a/fs/hfsplus/brec.c > > > +++ b/fs/hfsplus/brec.c > > > @@ -73,9 +73,10 @@ int hfs_brec_insert(struct hfs_find_data *fd, void *entry, int entry_len) > > > if (!fd->bnode) { > > > if (!tree->root) > > > hfs_btree_inc_height(tree); > > > - fd->bnode = hfs_bnode_find(tree, tree->leaf_head); > > > - if (IS_ERR(fd->bnode)) > > > - return PTR_ERR(fd->bnode); > > > > > > Are you sure that no caller is used this error code? Did you check this? > > > > Maybe, it makes sense to extract the error code and to show the error > > message on the caller side instead of processing the simple NULL? > > > > No response? Could we please get this wrapped up? I'm sorry, I thought you had picked this up already. Yes, I did check that no caller was using this. fd->bnode is always assumed to be NULL on error. Also, the error code is not lost, it's the return value of the function.
On Wed, 2018-08-22 at 15:11 -0300, Ernesto A. Fernández wrote: > On Tue, Aug 21, 2018 at 04:02:24PM -0700, Andrew Morton wrote: > > On Mon, 02 Jul 2018 11:01:37 -0700 Viacheslav Dubeyko <slava@dubeyko.com> wrote: > > > > > On Fri, 2018-06-29 at 15:34 -0300, Ernesto A. Fernández wrote: > > > > The hfs_find_exit() function expects fd->bnode to be NULL after a > > > > search has failed. The hfs_brec_insert() function may instead set > > > > it to an error-valued pointer. Fix this to prevent a crash. > > > > > > > > Reported-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com> > > > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > > > > --- > > > > fs/hfsplus/brec.c | 7 ++++--- > > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c > > > > index 808f4d8c859c..ed8eacb34452 100644 > > > > --- a/fs/hfsplus/brec.c > > > > +++ b/fs/hfsplus/brec.c > > > > @@ -73,9 +73,10 @@ int hfs_brec_insert(struct hfs_find_data *fd, void *entry, int entry_len) > > > > if (!fd->bnode) { > > > > if (!tree->root) > > > > hfs_btree_inc_height(tree); > > > > - fd->bnode = hfs_bnode_find(tree, tree->leaf_head); > > > > - if (IS_ERR(fd->bnode)) > > > > - return PTR_ERR(fd->bnode); > > > > > > > > > Are you sure that no caller is used this error code? Did you check this? > > > > > > Maybe, it makes sense to extract the error code and to show the error > > > message on the caller side instead of processing the simple NULL? > > > > > > > No response? Could we please get this wrapped up? > > I'm sorry, I thought you had picked this up already. Yes, I did check that > no caller was using this. fd->bnode is always assumed to be NULL on error. > Also, the error code is not lost, it's the return value of the function. OK. Looks reasonable. Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com> Thanks, Vyacheslav Dubeyko.
diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c index 808f4d8c859c..ed8eacb34452 100644 --- a/fs/hfsplus/brec.c +++ b/fs/hfsplus/brec.c @@ -73,9 +73,10 @@ int hfs_brec_insert(struct hfs_find_data *fd, void *entry, int entry_len) if (!fd->bnode) { if (!tree->root) hfs_btree_inc_height(tree); - fd->bnode = hfs_bnode_find(tree, tree->leaf_head); - if (IS_ERR(fd->bnode)) - return PTR_ERR(fd->bnode); + node = hfs_bnode_find(tree, tree->leaf_head); + if (IS_ERR(node)) + return PTR_ERR(node); + fd->bnode = node; fd->record = -1; } new_node = NULL;
The hfs_find_exit() function expects fd->bnode to be NULL after a search has failed. The hfs_brec_insert() function may instead set it to an error-valued pointer. Fix this to prevent a crash. Reported-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> --- fs/hfsplus/brec.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)