hfsplus: fix segfault when deleting all attrs of a file
Subsystems (2)
PATCH 1/1 hfsplus: fix segfault when deleting all attrs of a file
A segmentation fault can be triggered by setting many xattrs to a file and then deleting it. The number must be high enough for more than one b-tree node to be needed for storage.
When hfs_brec_remove() is called as part of hfsplus_delete_all_attrs(), fd->search_key will not be set to any specific value. It does not matter because we intend to remove all records for a given cnid.
The problem is that hfs_brec_remove() assumes it is being called with the result of a search by key, not by cnid. The value of search_key may be used to update the parent nodes. When no appropriate parent record is found, the result is an out of bounds access.
To fix this, set the value of fd->search_key to the key of the first record in the node, which is also the key of the corresponding parent record.
--- fs/hfsplus/brec.c | 3 +++ 1 file changed, 3 insertions(+)
fs/hfsplus/brec.c +3 -0
@@ -182,6 +182,9 @@ int hfs_brec_remove(struct hfs_find_data *fd) tree = fd->tree; node = fd->bnode; + +/* in case we need to search the parent node */ +hfs_bnode_read_key (node, fd->search_key,14 ); again: rec_off = tree->node_size - (fd->record +2 ) *2 ; end_off = tree->node_size - (node->num_recs +1 ) *2 ;
Signature · 1 line
2.1.4
Viacheslav Dubeyko
maintainerQuoted 6 lines · Ernesto A. Fernández
A segmentation fault can be triggered by setting many xattrs to a file
and then deleting it. The number must be high enough for more than one
b-tree node to be needed for storage.
I think it could be great to see in the description:
(1) The explanation of the reproducing path of the issue;
(2) The callstack of the crash and more details about it (if you are able to reproduce the issue, of course).
Could you please add these details in the description?
Quoted 5 lines
When
hfs_brec_remove()is called as part ofhfsplus_delete_all_attrs(),fd->search_keywill not be set to any specific value. It does not matter
because we intend to remove all records for a given cnid.
I am not fully follow to the issue. The hfsplus_delete_all_attrs calls hfsplus_find_attr() before hfsplus_delete_attr():
for (;;) {
err = hfsplus_find_attr(dir->i_sb, cnid, NULL, &fd);
if (err) {
if (err != -ENOENT)
pr_err("xattr search failed\n");
goto end_delete_all;
}
err = __hfsplus_delete_attr(dir, cnid, &fd);
if (err)
goto end_delete_all;
}The hfsplus_find_attr() prepares fd->search_key:
if (name) {
err = hfsplus_attr_build_key(sb, fd->search_key, cnid, name);
if (err)
goto failed_find_attr;
err = hfs_brec_find(fd, hfs_find_rec_by_key);
if (err)
goto failed_find_attr;
} else {
err = hfsplus_attr_build_key(sb, fd->search_key, cnid, NULL);
if (err)
goto failed_find_attr;
err = hfs_brec_find(fd, hfs_find_1st_rec_by_cnid);
if (err)
goto failed_find_attr;
}And, finally, __hfsplus_delete_attr() calls the hfs_brec_remove(). I am not follow to the explanation of the issue. Maybe I missed something? Could you share the callstack of the crash andd more details?
Quoted 25 lines
The problem is that
hfs_brec_remove()assumes it is being called with the result of a search by key, not by cnid. The value ofsearch_keymay
be used to update the parent nodes. When no appropriate parent record is
found, the result is an out of bounds access.To fix this, set the value of
fd->search_keyto the key of the first record in the node, which is also the key of the corresponding parent record.Signed-off-by Ernesto A. Fernández <[email protected]>---
fs/hfsplus/brec.c | 3 +++1 file changed, 3 insertions(+)fs/hfsplus/brec.c +0
-0@@ -182,6 +182,9 @@ int hfs_brec_remove(struct hfs_find_data *fd)
tree =fd->tree;
node =fd->bnode;
fs/hfsplus/brec.c int hfs_brec_remove(struct hfs_find_data *fd) · line 187
Quoted 3 lines
fs/hfsplus/brec.c +3
-0+ +/* in case we need to search the parent node */ +hfs_bnode_read_key (node, fd->search_key,14 );
The hardcoded value looks really weird. Could you rework this by means of adding some constant with reasonable name? Why 14?
Quoted 3 lines
again:
rec_off=tree->node_size- (fd->record+ 2) * 2;end_off=tree->node_size- (node->num_recs+ 1) * 2;
Thanks,
Vyacheslav Dubeyko.
Quoted 8 lines · Viacheslav Dubeyko
Quoted 6 lines · Ernesto A. Fernández
A segmentation fault can be triggered by setting many xattrs to a file
and then deleting it. The number must be high enough for more than one
b-tree node to be needed for storage.
Quoted 6 lines
I think it could be great to see in the description:
(1) The explanation of the reproducing path of the issue;
(2) The callstack of the crash and more details about it (if you are able to reproduce the issue, of course).Could you please add these details in the description?
I sent the script I used in a separate mail, would that be enough help? It seems from your review that you already understood all that the callstack can tell you. Maybe I should add the script to the commit message?
Quoted 26 lines
Quoted 6 lines
When
hfs_brec_remove()is called as part ofhfsplus_delete_all_attrs(),fd->search_keywill not be set to any specific value. It does not matter
because we intend to remove all records for a given cnid.I am not fully follow to the issue. The
hfsplus_delete_all_attrscallshfsplus_find_attr()beforehfsplus_delete_attr():for (;;) { err = hfsplus_find_attr(dir->i_sb, cnid, NULL, &fd); if (err) { if (err != -ENOENT) pr_err("xattr search failed\n"); goto end_delete_all; } err = __hfsplus_delete_attr(dir, cnid, &fd); if (err) goto end_delete_all; }The
hfsplus_find_attr()prepares fd->search_key:
Quoted 9 lines
if (name) {
err = hfsplus_attr_build_key(sb, fd->search_key, cnid, name);
if (err)
goto failed_find_attr;
err = hfs_brec_find(fd, hfs_find_rec_by_key);
if (err)
goto failed_find_attr;
} else {
err = hfsplus_attr_build_key(sb, fd->search_key, cnid, NULL);Here hfsplus_attr_build_key will only prepare the key for a search by cnid. We do not yet know the name of the xattr (that's what the NULL is) so it can't be set to anything specific.
Quoted 9 lines
if (err) goto failed_find_attr; err = hfs_brec_find(fd, hfs_find_1st_rec_by_cnid); if (err) goto failed_find_attr; }And, finally,
__hfsplus_delete_attr()calls thehfs_brec_remove(). I am not follow to the explanation of the issue. Maybe I missed something?
The problem is that hfs_brec_remove() assumes that fd was prepared for a search by key, but it was prepared for a search by cnid. It will fail to find the parent records.
Quoted 19 lines
Could you share the callstack of the crash andd more details?
Quoted 17 lines
The problem is that
hfs_brec_remove()assumes it is being called with the result of a search by key, not by cnid. The value ofsearch_keymay
be used to update the parent nodes. When no appropriate parent record is
found, the result is an out of bounds access.To fix this, set the value of
fd->search_keyto the key of the first record in the node, which is also the key of the corresponding parent record.Signed-off-by Ernesto A. Fernández <[email protected]>---
fs/hfsplus/brec.c | 3 +++1 file changed, 3 insertions(+)
Quoted 14 lines
fs/hfsplus/brec.c int hfs_brec_remove(struct hfs_find_data *fd) · line 187
Quoted 11 lines
fs/hfsplus/brec.c +0
-0@@ -182,6 +182,9 @@ int hfs_brec_remove(struct hfs_find_data *fd)
tree =fd->tree;
node =fd->bnode;fs/hfsplus/brec.c +3
-0+ +/* in case we need to search the parent node */ +hfs_bnode_read_key (node, fd->search_key,14 );The hardcoded value looks really weird. Could you rework this by means of adding some constant with reasonable name? Why 14?
I agree that it's weird, but the number 14 is all over brec.c. It would be more confusing if I used a constant only this one time, so I decided to respect the style that was in use.
To be clear, 14 is sizeof(hfs_bnode_desc), that is, the size of the header of the bnode, and the position of the first record. If you find it better, maybe we can change 14 to sizeof(hfs_bnode_desc) every time it appears in the file, but I think it should be a separate patch.
Quoted 9 lines
Quoted 4 lines
again:
rec_off=tree->node_size- (fd->record+ 2) * 2;end_off=tree->node_size- (node->num_recs+ 1) * 2;Thanks,
Vyacheslav Dubeyko.
Thank you for your review,
Ernest
Viacheslav Dubeyko
maintainerQuoted 17 lines · Ernesto A. Fernández
Quoted 16 lines · Viacheslav Dubeyko
Quoted 8 lines · Ernesto A. Fernández
A segmentation fault can be triggered by setting many xattrs to a file
and then deleting it. The number must be high enough for more than
one
b-tree node to be needed for storage.I think it could be great to see in the description:
(1) The explanation of the reproducing path of the issue;
(2) The callstack of the crash and more details about it (if you are
able to reproduce the issue, of course).
Quoted 7 lines
Quoted 1 line
Could you please add these details in the description?
I sent the script I used in a separate mail, would that be enough help?
It seems from your review that you already understood all that the callstack can tell you. Maybe I should add the script to the commit message?
The script is the way to reproduce the issue. But the callstack of the crash will be the explanation of the symptoms and the environment of the issue. Also the callstack provides the opportunity to analyze the issue.
Quoted 57 lines
Quoted 38 lines
Quoted 8 lines
When
hfs_brec_remove()is called as part ofhfsplus_delete_all_attrs(),fd->search_keywill not be set to any specific value. It does not matter
because we intend to remove all records for a given cnid.I am not fully follow to the issue. The
hfsplus_delete_all_attrscallshfsplus_find_attr()beforehfsplus_delete_attr():for (;;) { err = hfsplus_find_attr(dir->i_sb, cnid, NULL, &fd); if (err) { if (err != -ENOENT) pr_err("xattr search failed\n"); goto end_delete_all; } err = __hfsplus_delete_attr(dir, cnid, &fd); if (err) goto end_delete_all; }The
hfsplus_find_attr()prepares fd->search_key:if (name) { err = hfsplus_attr_build_key(sb, fd->search_key, cnid, name); if (err) goto failed_find_attr; err = hfs_brec_find(fd, hfs_find_rec_by_key); if (err) goto failed_find_attr; } else { err = hfsplus_attr_build_key(sb, fd->search_key, cnid,Quoted 1 line
NULL);
Here
hfsplus_attr_build_keywill only prepare the key for a search by cnid.
We do not yet know the name of the xattr (that's what the NULL is) so it
can't be set to anything specific.Quoted 8 lines
if (err) goto failed_find_attr; err = hfs_brec_find(fd, hfs_find_1st_rec_by_cnid); if (err) goto failed_find_attr; }Quoted 4 lines
And, finally,
__hfsplus_delete_attr()calls thehfs_brec_remove(). I am
not follow to the explanation of the issue. Maybe I missed something?
Quoted 6 lines
The problem is that
hfs_brec_remove()assumes that fd was prepared for a
search by key, but it was prepared for a search by cnid. It will fail to
find the parent records.
I still cannot follow what the real cause of the issue. This is why I requested the callstack of the crash. Could you point out the code line in hfs_brec_remove() that is the place of the crash? I don't see the place where hfs_brec_remove() is trying to access the fd->search_key directly. It sounds for me that the real trouble is not in the hfs_brec_remove().
Let's imagine that we have trouble with fd->search_key in the hfs_brec_remove(). But then it means that the real fix should be in hfs_brec_find() or hfs_find_1st_rec_by_cnid(). But, currently, I am not convinced by your explanation that the reason is there where you did fix it.
The struct hfsplus_attr_key is simply the piece of memory:
struct hfsplus_attr_key {
__be16 key_len;
__be16 pad;
hfsplus_cnid cnid;
__be32 start_block;
struct hfsplus_attr_unistr key_name;
} __packed;And the hfsplus_attr_build_key() initializes this piece of memory:
int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
u32 cnid, const char *name)
{
int len;
memset(key, 0, sizeof(struct hfsplus_attr_key));
key->attr.cnid = cpu_to_be32(cnid);
if (name) {
int res = hfsplus_asc2uni(sb,
(struct hfsplus_unistr *)&key->attr.key_name,
HFSPLUS_ATTR_MAX_STRLEN, name, strlen(name));
if (res)
return res;
len = be16_to_cpu(key->attr.key_name.length);
} else {
key->attr.key_name.length = 0;
len = 0;
}
/* The length of the key, as stored in key_len field, does not include
* the size of the key_len field itself.
* So, offsetof(hfsplus_attr_key, key_name) is a trick because
* it takes into consideration key_len field (__be16) of
* hfsplus_attr_key structure instead of length field (__be16) of
* hfsplus_attr_unistr structure.
*/
key->key_len =
cpu_to_be16(offsetof(struct hfsplus_attr_key, key_name) +
2 * len);
return 0;
}So, even if we haven't name in this structure then the attr.key_name will have zero length and the buffer will contain the string is filled by zeros. And it's not clear for me how this can be the reason of crash? Could you share the callstack of the crash, finally? I still don't follow your vision. Please, explain the issue and your fix in more clear and detailed way.
It is possible to do the search by name only or by cnid only. Everything should work OK. And I didn't rememer any troubles with hfsplus_delete_all_attrs() on the implementation and testing phase. I think you could have some specific environment or maybe the code base was changed dramatically. But, anyway, I don't follow your vision of the issue.
Quoted 47 lines
Quoted 27 lines
Could you share the callstack of the crash andd more details?
Quoted 24 lines
The problem is that
hfs_brec_remove()assumes it is being called with
the result of a search by key, not by cnid. The value ofsearch_key
may
be used to update the parent nodes. When no appropriate parent record
is
found, the result is an out of bounds access.To fix this, set the value of
fd->search_keyto the key of the first
record in the node, which is also the key of the corresponding parent
record.Signed-off-by Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
fs/hfsplus/brec.c | 3 +++1 file changed, 3 insertions(+)Quoted 14 lines
fs/hfsplus/brec.c int hfs_brec_remove(struct hfs_find_data *fd) · line 187
Quoted 11 lines
fs/hfsplus/brec.c +0
-0@@ -182,6 +182,9 @@ int hfs_brec_remove(struct hfs_find_data *fd)
tree =fd->tree;
node =fd->bnode;fs/hfsplus/brec.c +3
-0+ +/* in case we need to search the parent node */ +hfs_bnode_read_key (node, fd->search_key,14 );The hardcoded value looks really weird. Could you rework this by means
of adding some constant with reasonable name? Why 14?I agree that it's weird, but the number 14 is all over
brec.c. It would be
more confusing if I used a constant only this one time, so I decided to
respect the style that was in use.
Quoted 7 lines
To be clear, 14 is
sizeof(hfs_bnode_desc), that is, the size of the header of the bnode, and the position of the first record. If you find
it better, maybe we can change 14 tosizeof(hfs_bnode_desc)every time it appears in the file, but I think it should be a separate patch.
The using of sizeof(hfs_bnode_desc) will be pretty good and clean solution.
Thanks,
Vyacheslav Dubeyko.
Quoted 26 lines · Viacheslav Dubeyko
Quoted 17 lines · Ernesto A. Fernández
Quoted 16 lines · Viacheslav Dubeyko
Quoted 8 lines · Ernesto A. Fernández
A segmentation fault can be triggered by setting many xattrs to a file
and then deleting it. The number must be high enough for more than
one
b-tree node to be needed for storage.I think it could be great to see in the description:
(1) The explanation of the reproducing path of the issue;
(2) The callstack of the crash and more details about it (if you are
able to reproduce the issue, of course).Quoted 7 lines
Quoted 1 line
Could you please add these details in the description?
I sent the script I used in a separate mail, would that be enough help?
It seems from your review that you already understood all that the callstack can tell you. Maybe I should add the script to the commit message?
Quoted 4 lines
The script is the way to reproduce the issue. But the callstack of the crash will be the explanation of the symptoms and the environment of the issue. Also the callstack provides the opportunity to analyze the issue.
All right, I'll include it at the end of the mail.
Quoted 64 lines
Quoted 57 lines
Quoted 38 lines
Quoted 8 lines
When
hfs_brec_remove()is called as part ofhfsplus_delete_all_attrs(),fd->search_keywill not be set to any specific value. It does not matter
because we intend to remove all records for a given cnid.I am not fully follow to the issue. The
hfsplus_delete_all_attrscallshfsplus_find_attr()beforehfsplus_delete_attr():for (;;) { err = hfsplus_find_attr(dir->i_sb, cnid, NULL, &fd); if (err) { if (err != -ENOENT) pr_err("xattr search failed\n"); goto end_delete_all; } err = __hfsplus_delete_attr(dir, cnid, &fd); if (err) goto end_delete_all; }The
hfsplus_find_attr()prepares fd->search_key:if (name) { err = hfsplus_attr_build_key(sb, fd->search_key, cnid, name); if (err) goto failed_find_attr; err = hfs_brec_find(fd, hfs_find_rec_by_key); if (err) goto failed_find_attr; } else { err = hfsplus_attr_build_key(sb, fd->search_key, cnid,Quoted 1 line
NULL);
Here
hfsplus_attr_build_keywill only prepare the key for a search by cnid.
We do not yet know the name of the xattr (that's what the NULL is) so it
can't be set to anything specific.Quoted 8 lines
if (err) goto failed_find_attr; err = hfs_brec_find(fd, hfs_find_1st_rec_by_cnid); if (err) goto failed_find_attr; }Quoted 4 lines
And, finally,
__hfsplus_delete_attr()calls thehfs_brec_remove(). I am
not follow to the explanation of the issue. Maybe I missed something?Quoted 6 lines
The problem is that
hfs_brec_remove()assumes that fd was prepared for a
search by key, but it was prepared for a search by cnid. It will fail to
find the parent records.
Quoted 3 lines
I still cannot follow what the real cause of the issue. This is why I requested the callstack of the crash. Could you point out the code line in
hfs_brec_remove()that is the place of the crash?
Sure, the crash happens in line 219, when the out of bounds write takes place. But that doesn't tell you much, because the real issue here is that fd->record has been set to -1 before this call. That value means that no record has been found, but since that makes no sense (in this context) the hfs_brec_remove() code acts as if it was an actual record and tries to write to it.
Quoted 4 lines
I don't see the
place wherehfs_brec_remove()is trying to access the fd->search_key directly. It sounds for me that the real trouble is not in thehfs_brec_remove().
It is used directly in line 206, explicitly searching the parent node by key when the search_key has not been set for that purpose. It also uses it indirectly in line 229, again to search the parent records. In the second case this just means that we will fail to update the parent record, which is bad enough. But the first case is the one that will set fd->record to -1 before going through the code again, this time for the parent node. When hfs_bnode_write_u16() tries to delete this "-1 record" on the parent, there is an out of bounds write.
This last thing only happens when we have just deleted the last record of a bnode.
Quoted 5 lines
Let's imagine that we have trouble with
fd->search_keyin thehfs_brec_remove(). But then it means that the real fix should be in hfs_brec_find() or hfs_find_1st_rec_by_cnid(). But, currently, I am not convinced by your explanation that the reason is there where you did fix it.
I thought about having hfs_brec_find() set fd->search_key to the same value as fd->key before returning, but I think that would be wrong. The caller may expect the search_key to remain the same after the search, and even reuse it. In fact that's what the hfs_brec_remove() function is trying to do, in a buggy style.
Quoted 45 lines
The struct
hfsplus_attr_keyis simply the piece of memory:struct hfsplus_attr_key { __be16 key_len; __be16 pad; hfsplus_cnid cnid; __be32 start_block; struct hfsplus_attr_unistr key_name; } __packed;And the hfsplus_attr_build_key() initializes this piece of memory:
int hfsplus_attr_build_key(struct
super_block*sb,hfsplus_btree_key*key,
u32 cnid, const char *name){ int len; memset(key, 0, sizeof(struct hfsplus_attr_key)); key->attr.cnid = cpu_to_be32(cnid); if (name) { int res = hfsplus_asc2uni(sb, (struct hfsplus_unistr *)&key->attr.key_name, HFSPLUS_ATTR_MAX_STRLEN, name, strlen(name)); if (res) return res; len = be16_to_cpu(key->attr.key_name.length); } else { key->attr.key_name.length = 0; len = 0; } /* The length of the key, as stored in key_len field, does not include * the size of the key_len field itself. * So, offsetof(hfsplus_attr_key, key_name) is a trick because * it takes into consideration key_len field (__be16) of * hfsplus_attr_key structure instead of length field (__be16) of * hfsplus_attr_unistr structure. */ key->key_len = cpu_to_be16(offsetof(struct hfsplus_attr_key, key_name) + 2 * len); return 0; }
Quoted 6 lines
So, even if we haven't name in this structure then the attr.key_name will have zero length and the buffer will contain the string is filled by zeros. And it's not clear for me how this can be the reason of crash? Could you share the callstack of the crash, finally? I still don't follow your vision. Please, explain the issue and your fix in more clear and detailed way.
I haven't checked the key comparison function used, but what I see in practice is that when you use a search_key that was set in this way (prepared only to find the first record with a given cnid) for the purpose of a search by key, the result is not correct. I get fd->record == -1 in my case, but maybe it can fail in different ways in other situations. I thought this was clearly a bug in the caller, that had not set the key to the right value. You seem to think that the search should work anyway, I believe?
Quoted 3 lines
It is possible to do the search by name only or by cnid only. Everything should work OK.
I don't think you can search by name only. You can search by key, and the key has both the name and the cnid. The search by cnid is for finding the first record with a given cnid, you don't yet know the name so you can't search by key. This is used by hfsplus_delete_all_attrs() because it doesn't care about which record is deleted first, it simply deletes all with a given cnid.
Also you have to explicitly state in the call to __hfs_brec_find() which one of the two search strategies you will be using. I don't think it is reasonable to expect __hfs_brec_find() to give a valid result if it is called with hfs_find_rec_by_key but fd->search_key is only prepared for a search by cnid.
Quoted 5 lines
And I didn't rememer any troubles
with hfsplus_delete_all_attrs() on the implementation and testing phase. I think you could have some specific environment or maybe the code base was changed dramatically. But, anyway, I don't follow your vision of the issue.
You are probably right here: I've done all my testing in a fresh filesystem, and I realize now this segfault would be harder to trigger if the filesystem has been aged, because the xattrs of different files may share a single bnode. So deleting all xattrs of a single file would not necessarily delete a parent bnode, no matter how many xattrs.
This bug would still have consequences though, but they may be more subtle.
Quoted 55 lines
Quoted 47 lines
Quoted 27 lines
Could you share the callstack of the crash andd more details?
Quoted 24 lines
The problem is that
hfs_brec_remove()assumes it is being called with
the result of a search by key, not by cnid. The value ofsearch_key
may
be used to update the parent nodes. When no appropriate parent record
is
found, the result is an out of bounds access.To fix this, set the value of
fd->search_keyto the key of the first
record in the node, which is also the key of the corresponding parent
record.Signed-off-by Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
fs/hfsplus/brec.c | 3 +++1 file changed, 3 insertions(+)Quoted 14 lines
fs/hfsplus/brec.c int hfs_brec_remove(struct hfs_find_data *fd) · line 187
Quoted 11 lines
fs/hfsplus/brec.c +0
-0@@ -182,6 +182,9 @@ int hfs_brec_remove(struct hfs_find_data *fd)
tree =fd->tree;
node =fd->bnode;fs/hfsplus/brec.c +3
-0+ +/* in case we need to search the parent node */ +hfs_bnode_read_key (node, fd->search_key,14 );The hardcoded value looks really weird. Could you rework this by means
of adding some constant with reasonable name? Why 14?I agree that it's weird, but the number 14 is all over
brec.c. It would be
more confusing if I used a constant only this one time, so I decided to
respect the style that was in use.Quoted 7 lines
To be clear, 14 is
sizeof(hfs_bnode_desc), that is, the size of the header of the bnode, and the position of the first record. If you find
it better, maybe we can change 14 tosizeof(hfs_bnode_desc)every time it appears in the file, but I think it should be a separate patch.
Quoted 8 lines
The using of sizeof(hfs_bnode_desc) will be pretty good and clean solution.
Thanks,
Vyacheslav Dubeyko.
Here's the callstack, I hope it helps you:
[ 3550.503259] general protection fault: 0000 [#1] SMP [ 3550.503587] Modules linked in: nls_utf8 hfsplus loop nfsd auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc dlm configfs ppdev snd_pcm snd_timer cirrus snd soundcore ttm pcspkr evdev parport_pc parport pvpanic serio_raw drm_kms_helper button 9pnet_virtio 9pnet drm autofs4 xfs libcrc32c sg sr_mod sd_mod cdrom ata_generic ata_piix libata crc32c_intel psmouse virtio_pci virtio_ring virtio e1000 i2c_piix4 i2c_core scsi_mod floppy [ 3550.504013] CPU: 0 PID: 1072 Comm: rm Not tainted 4.14.0-rc3+ #16 [ 3550.504013] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014 [ 3550.504013] task: ffff880058ab4000 task.stack: ffffc90001630000 [ 3550.504013] RIP: 0010:hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] [ 3550.504013] RSP: 0018:ffffc90001633c08 EFLAGS: 00010202 [ 3550.504013] RAX: 0005100000000000 RBX: 0000000000000002 RCX: 00000000000000ff [ 3550.504013] RDX: 0000000000000000 RSI: ffffc90001633c56 RDI: ffff88002972d780 [ 3550.504013] RBP: ffffc90001633c40 R08: ffff88002972d790 R09: 0000000000000000 [ 3550.504013] R10: 0000000000000006 R11: 0000000000000002 R12: 0000000000000002 [ 3550.504013] R13: 0000000000000002 R14: ffff88002972d7e0 R15: ffffc90001633c56 [ 3550.504013] FS: 00007fc27f0c8700(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000 [ 3550.504013] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 3550.504013] CR2: 00007fc27ec40450 CR3: 00000000584c6000 CR4: 00000000000006f0 [ 3550.504013] Call Trace: [ 3550.504013] hfsplus_bnode_write_u16+0x27/0x30 [hfsplus] [ 3550.504013] hfsplus_brec_remove+0x117/0x170 [hfsplus] [ 3550.504013] __hfsplus_delete_attr+0x94/0xf0 [hfsplus] [ 3550.504013] hfsplus_delete_all_attrs+0x4a/0xb0 [hfsplus] [ 3550.504013] hfsplus_delete_cat+0x1f5/0x300 [hfsplus] [ 3550.504013] hfsplus_unlink+0x82/0x1e0 [hfsplus] [ 3550.504013] ? __inode_permission+0x44/0xc0 [ 3550.504013] vfs_unlink+0xf1/0x180 [ 3550.504013] do_unlinkat+0x25f/0x2e0 [ 3550.504013] SyS_unlinkat+0x1b/0x30 [ 3550.504013] entry_SYSCALL_64_fastpath+0x1e/0xa9 [ 3550.504013] RIP: 0033:0x7fc27ebe632d [ 3550.504013] RSP: 002b:00007fff036b4008 EFLAGS: 00000202 ORIG_RAX: 0000000000000107 [ 3550.504013] RAX: ffffffffffffffda RBX: 00000000025cd2f0 RCX: 00007fc27ebe632d [ 3550.504013] RDX: 0000000000000000 RSI: 00000000025cc0c0 RDI: ffffffffffffff9c [ 3550.504013] RBP: 00000000025cd420 R08: 0000000000000003 R09: 0000000000000000 [ 3550.504013] R10: 00007fff036b3dd0 R11: 0000000000000202 R12: 00000000025cc030 [ 3550.504013] R13: 00000000025cd3f8 R14: 0000000000000000 R15: 0000000000000000 [ 3550.504013] Code: c1 fb 06 48 c1 e3 0c 48 01 d8 49 63 dd 48 01 d0 48 83 fb 08 73 26 f6 c3 04 0f 85 04 01 00 00 48 85 db 74 44 41 0f b6 0f f6 c3 02 <88> 08 74 39 41 0f b7 4c 1f fe 66 89 4c 03 fe eb 2c 49 8b 0f 48 [ 3550.504013] RIP: hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] RSP: ffffc90001633c08 [ 3550.540759] ---[ end trace 142de398139577f1 ]---
Viacheslav Dubeyko
maintainerQuoted 58 lines · Ernesto A. Fernández
Quoted 3 lines
Here's the callstack, I hope it helps you:
[ 3550.503259] general protection fault: 0000 [#1] SMP [ 3550.503587] Modules linked in: nls_utf8 hfsplus loop nfsd
auth_rpcgssoid_registrynfs_aclnfs lockd grace fscache sunrpc dlm configfs ppdevsnd_pcmsnd_timercirrus snd soundcore ttm pcspkr evdevparport_pcparport pvpanicserio_rawdrm_kms_helperbutton 9pnet_virtio 9pnet drm autofs4 xfs libcrc32c sgsr_modsd_modcdromata_genericata_piixlibatacrc32c_intelpsmousevirtio_pcivirtio_ringvirtio e1000i2c_piix4i2c_corescsi_modfloppy[ 3550.504013] CPU: 0 PID: 1072 Comm: rm Not tainted 4.14.0-rc3+ #16 [ 3550.504013] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014 [ 3550.504013] task: ffff880058ab4000 task.stack: ffffc90001630000 [ 3550.504013] RIP: 0010:hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] [ 3550.504013] RSP: 0018:ffffc90001633c08 EFLAGS: 00010202 [ 3550.504013] RAX: 0005100000000000 RBX: 0000000000000002 RCX: 00000000000000ff [ 3550.504013] RDX: 0000000000000000 RSI: ffffc90001633c56 RDI: ffff88002972d780 [ 3550.504013] RBP: ffffc90001633c40 R08: ffff88002972d790 R09: 0000000000000000 [ 3550.504013] R10: 0000000000000006 R11: 0000000000000002 R12: 0000000000000002 [ 3550.504013] R13: 0000000000000002 R14: ffff88002972d7e0 R15: ffffc90001633c56 [ 3550.504013] FS: 00007fc27f0c8700(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000 [ 3550.504013] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 3550.504013] CR2: 00007fc27ec40450 CR3: 00000000584c6000 CR4: 00000000000006f0 [ 3550.504013] Call Trace: [ 3550.504013] hfsplus_bnode_write_u16+0x27/0x30 [hfsplus] [ 3550.504013] hfsplus_brec_remove+0x117/0x170 [hfsplus] [ 3550.504013] __hfsplus_delete_attr+0x94/0xf0 [hfsplus] [ 3550.504013] hfsplus_delete_all_attrs+0x4a/0xb0 [hfsplus] [ 3550.504013] hfsplus_delete_cat+0x1f5/0x300 [hfsplus] [ 3550.504013] hfsplus_unlink+0x82/0x1e0 [hfsplus] [ 3550.504013] ? __inode_permission+0x44/0xc0 [ 3550.504013] vfs_unlink+0xf1/0x180 [ 3550.504013] do_unlinkat+0x25f/0x2e0 [ 3550.504013] SyS_unlinkat+0x1b/0x30 [ 3550.504013] entry_SYSCALL_64_fastpath+0x1e/0xa9 [ 3550.504013] RIP: 0033:0x7fc27ebe632d [ 3550.504013] RSP: 002b:00007fff036b4008 EFLAGS: 00000202 ORIG_RAX: 0000000000000107 [ 3550.504013] RAX: ffffffffffffffda RBX: 00000000025cd2f0 RCX: 00007fc27ebe632d [ 3550.504013] RDX: 0000000000000000 RSI: 00000000025cc0c0 RDI: ffffffffffffff9c [ 3550.504013] RBP: 00000000025cd420 R08: 0000000000000003 R09: 0000000000000000 [ 3550.504013] R10: 00007fff036b3dd0 R11: 0000000000000202 R12: 00000000025cc030 [ 3550.504013] R13: 00000000025cd3f8 R14: 0000000000000000 R15:
Quoted 8 lines
0000000000000000
[ 3550.504013] Code: c1 fb 06 48 c1 e3 0c 48 01 d8 49 63 dd 48 01 d0 48 83 fb 08 73 26 f6 c3 04 0f 85 04 01 00 00 48 85 db 74 44 41 0f b6 0f f6 c3 02 <88> 08 74 39 41 0f b7 4c 1f fe 66 89 4c 03 fe eb 2c 49 8b 0f 48 [ 3550.504013] RIP: hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] RSP: ffffc90001633c08 [ 3550.540759] ---[ end trace 142de398139577f1 ]---
Great. Thank you.
I can reproduce the crash. Let me check the issue.
Thanks,
Vyacheslav Dubeyko.
-----Original Message-----
From: Ernesto A. Fernández [mailto:[email protected]]
Sent: Monday, October 9, 2017 1:00 PM
To: Viacheslav Dubeyko <[email protected]>; [email protected]
Cc: Sergei Antonov <[email protected]>; Hin-Tak Leung <[email protected]>; Al Viro <[email protected]>; Christoph Hellwig <[email protected]>; Slava Dubeyko <[email protected]>; Ernesto A. Fernández <[email protected]>
Subject: Re: [PATCH] hfsplus: fix segfault when deleting all attrs of a file
Quoted 31 lines
Here's the callstack, I hope it helps you:
[ 3550.503259] general protection fault: 0000 [#1] SMP [ 3550.503587] Modules linked in: nls_utf8 hfsplus loop nfsd auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc dlm configfs ppdev snd_pcm snd_timer cirrus snd soundcore ttm pcspkr evdev parport_pc parport pvpanic serio_raw drm_kms_helper button 9pnet_virtio 9pnet drm autofs4 xfs libcrc32c sg sr_mod sd_mod cdrom ata_generic ata_piix libata crc32c_intel psmouse virtio_pci virtio_ring virtio e1000 i2c_piix4 i2c_core scsi_mod floppy [ 3550.504013] CPU: 0 PID: 1072 Comm: rm Not tainted 4.14.0-rc3+ #16 [ 3550.504013] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014 [ 3550.504013] task: ffff880058ab4000 task.stack: ffffc90001630000 [ 3550.504013] RIP: 0010:hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] [ 3550.504013] RSP: 0018:ffffc90001633c08 EFLAGS: 00010202 [ 3550.504013] RAX: 0005100000000000 RBX: 0000000000000002 RCX: 00000000000000ff [ 3550.504013] RDX: 0000000000000000 RSI: ffffc90001633c56 RDI: ffff88002972d780 [ 3550.504013] RBP: ffffc90001633c40 R08: ffff88002972d790 R09: 0000000000000000 [ 3550.504013] R10: 0000000000000006 R11: 0000000000000002 R12: 0000000000000002 [ 3550.504013] R13: 0000000000000002 R14: ffff88002972d7e0 R15: ffffc90001633c56 [ 3550.504013] FS: 00007fc27f0c8700(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000 [ 3550.504013] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 3550.504013] CR2: 00007fc27ec40450 CR3: 00000000584c6000 CR4: 00000000000006f0 [ 3550.504013] Call Trace: [ 3550.504013] hfsplus_bnode_write_u16+0x27/0x30 [hfsplus] [ 3550.504013] hfsplus_brec_remove+0x117/0x170 [hfsplus] [ 3550.504013] __hfsplus_delete_attr+0x94/0xf0 [hfsplus] [ 3550.504013] hfsplus_delete_all_attrs+0x4a/0xb0 [hfsplus] [ 3550.504013] hfsplus_delete_cat+0x1f5/0x300 [hfsplus] [ 3550.504013] hfsplus_unlink+0x82/0x1e0 [hfsplus] [ 3550.504013] ? __inode_permission+0x44/0xc0 [ 3550.504013] vfs_unlink+0xf1/0x180 [ 3550.504013] do_unlinkat+0x25f/0x2e0 [ 3550.504013] SyS_unlinkat+0x1b/0x30 [ 3550.504013] entry_SYSCALL_64_fastpath+0x1e/0xa9 [ 3550.504013] RIP: 0033:0x7fc27ebe632d [ 3550.504013] RSP: 002b:00007fff036b4008 EFLAGS: 00000202 ORIG_RAX: 0000000000000107
Quoted 9 lines
[ 3550.504013] RAX: ffffffffffffffda RBX: 00000000025cd2f0 RCX: 00007fc27ebe632d [ 3550.504013] RDX: 0000000000000000 RSI: 00000000025cc0c0 RDI: ffffffffffffff9c [ 3550.504013] RBP: 00000000025cd420 R08: 0000000000000003 R09: 0000000000000000 [ 3550.504013] R10: 00007fff036b3dd0 R11: 0000000000000202 R12: 00000000025cc030 [ 3550.504013] R13: 00000000025cd3f8 R14: 0000000000000000 R15: 0000000000000000 [ 3550.504013] Code: c1 fb 06 48 c1 e3 0c 48 01 d8 49 63 dd 48 01 d0 48 83 fb 08 73 26 f6 c3 04 0f 85 04 01 00 00 48 85 db 74 44 41 0f b6 0f f6 c3 02 <88> 08 74 39 41 0f b7 4c 1f fe 66 89 4c 03 fe eb 2c 49 8b 0f 48 [ 3550.504013] RIP: hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] RSP: ffffc90001633c08 [ 3550.540759] ---[ end trace 142de398139577f1 ]---
The whole picture is here:
(1) hfsplus_find_attr()
[ 458.039682] hfsplus: find_attr: (null),18
(2) hfs_brec_find(fd, hfs_find_1st_rec_by_cnid)
[ 458.039683] hfsplus: put_node(8:1): 1 [ 458.039685] hfsplus: get_node(8:3): 82 [ 458.039686] hfsplus: __hfs_brec_find: fd->record 0, fd->keyoffset 14, fd->keylength 26, fd->entryoffset 40, fd->entrylength 4 [ 458.039687] hfsplus: put_node(8:3): 82 [ 458.039688] hfsplus: get_node(8:1): 1 [ 458.039690] hfsplus: __hfs_brec_find: fd->record 0, fd->keyoffset 14, fd->keylength 30, fd->entryoffset 44, fd->entrylength 16
(3) __hfsplus_delete_attr()
(4) hfs_brec_remove()
[ 458.039691] hfsplus: bnode: 1 [ 458.039692] hfsplus: 2, 0, -1, 1, 1 [ 458.039693] hfsplus: 14 (28) [ 458.039695] hfsplus: 60 [ 458.039697] hfsplus: remove_rec: 0, 46 [ 458.039697] hfsplus: remove_rec: num_recs 1 [ 458.039700] hfsplus: new_node(8:2): 1 [ 458.040614] hfsplus: put_node(8:2): 1 [ 458.040618] hfsplus: get_node(8:3): 82 [ 458.040619] hfsplus: put_node(8:1): 1 [ 458.040620] hfsplus: remove_node(8:1): 0 [ 458.040622] hfsplus: btree_free_node: 1 [ 458.040623] hfsplus: new_node(8:0): 1 [ 458.041096] hfsplus: put_node(8:0): 1 [ 458.041102] hfsplus: __hfs_brec_find: fd->record -1, fd->keyoffset 14, fd->keylength 26, fd->entryoffset 40, fd->entrylength 4 [ 458.041103] hfsplus: bnode: 3 [ 458.041104] hfsplus: 0, 0, 0, 2, 2 [ 458.041105] hfsplus: 14 (26,1) [ 458.041107] hfsplus: 44 (30,2) [ 458.041110] hfsplus: 78 [ 458.041112] hfsplus: remove_rec: -1, 30 [ 458.041113] hfsplus: remove_rec: num_recs 2 [ 458.041114] hfsplus: remove_rec: num_recs 1 [ 458.041117] hfsplus: remove_rec: rec_off 8190, end_off 8186 [ 458.041118] hfsplus: remove_rec: rec_off 8190, end_off 8186 [ 458.041119] hfsplus: remove_rec: data_off 14, size 30 [ 458.041143] general protection fault: 0000 [#1] SMP
The hfs_brec_find() found the pointer (#1) in index node #3 by means of hfs_find_1st_rec_by_cnid strategy. Then the hfs_brec_find() found the single record in the leaf node #1 on the basis of the same strategy. The most interesting is starting in hfs_brec_remove():
if (!--node->num_recs) {
hfs_bnode_unlink(node);
if (!node->parent)
return 0;
parent = hfs_bnode_find(tree, node->parent);
if (IS_ERR(parent))
return PTR_ERR(parent);
hfs_bnode_put(node);
node = fd->bnode = parent;
__hfs_brec_find(node, fd, hfs_find_rec_by_key);
goto again;
}The hfs_brec_remove() is trying to delete the single and latest record from the leaf node #1. Finally, the node->num_recs will be equal to zero after the increment operation and the condition directs the code inside of this execution path. So, first of all, the logic unlink the leaf node #2 from leaf node #1. Then the logic gets the parent node and to put the leaf node #1. And, finally, we are trying to find a record in the parent and index node #3 on the basis of hfs_find_rec_by_key strategy. We cannot find anything because the fd->search_key is not prepared for such search. It means that fd->record is equal to -1. The real issue takes place here:
hfs_bnode_write_u16(node, rec_off + 2, data_off - size);
because the data_off == 14 and size == 30. We have overflow here and it is the reason of the crash. But the data_off is lesser of size because of value of fd->record we selected improper position of the offset for the changing the value in the offsets table.
What should be done for the issue fix?
(1) We need to add the check condition here:
http://elixir.free-electrons.com/linux/latest/source/fs/hfsplus/brec.c#L217
do {
data_off = hfs_bnode_read_u16(node, rec_off);/*
* TODO: check condition here
* if (data_off < size)
* return error here!!!!!
*/
hfs_bnode_write_u16(node, rec_off + 2, data_off - size);
rec_off -= 2;
} while (rec_off >= end_off);(2) The logic of hfs_brec_remove() should be reworked here:
http://elixir.free-electrons.com/linux/latest/source/fs/hfsplus/brec.c#L197
/*
* TODO: rework the logic here
if (node->num_recs <= 0)
BUG(); /* it could be error message here */
else if (node->num_recs == 1)
__hfs_brec_find(node, fd, hfs_find_1st_rec_by_cnid); /* maybe check the error here */
memcpy(fd->search_key, fd->key, sizeof(hfsplus_btree_key));
--node->num_recs;*/
hfs_bnode_unlink(node);
if (!node->parent)
return 0;
parent = hfs_bnode_find(tree, node->parent);
if (IS_ERR(parent))
return PTR_ERR(parent);
hfs_bnode_put(node);
node = fd->bnode = parent;
__hfs_brec_find(node, fd, hfs_find_rec_by_key);
goto again;
}So, we need to extract the key of the latest record from the leaf node by using the hfs_find_1st_rec_by_cnid strategy, copy fd->key into the fd->search_key and then to go into logic of searching the record in the index node on the basis of hfs_find_rec_by_key strategy and the deletion of the record from the index node.
What do you think? Could you rework the patch?
Thanks,
Vyacheslav Dubeyko.
I really appreciate your feedback, but I would rather you spent more time trying to understand my work instead of redoing it from scratch. You are making many of the same mistakes I made when I was first tracking down this bug:
Quoted 27 lines · Slava Dubeyko
The
hfs_brec_find()found the pointer (#1) in index node #3 by means ofhfs_find_1st_rec_by_cnidstrategy. Then thehfs_brec_find()found the single record in the leaf node #1 on the basis of the same strategy. The most interesting is starting inhfs_brec_remove():if (!--node->num_recs) { hfs_bnode_unlink(node); if (!node->parent) return 0; parent = hfs_bnode_find(tree, node->parent); if (IS_ERR(parent)) return PTR_ERR(parent); hfs_bnode_put(node); node = fd->bnode = parent; __hfs_brec_find(node, fd, hfs_find_rec_by_key); goto again; }The
hfs_brec_remove()is trying to delete the single and latest record from the leaf node #1. Finally, thenode->num_recswill be equal to zero after the increment operation and the condition directs the code inside of this execution path. So, first of all, the logic unlink the leaf node #2 from leaf node #1. Then the logic gets the parent node and to put the leaf node #1. And, finally, we are trying to find a record in the parent and index node #3 on the basis ofhfs_find_rec_by_keystrategy. We cannot find anything because thefd->search_keyis not prepared for such search. It means thatfd->recordis equal to -1. The real issue takes place here:
Quoted 4 lines
hfs_bnode_write_u16(node, rec_off + 2, data_off - size);because the
data_off== 14 and size == 30. We have overflow here and it is the reason of the crash.
No. The reason of the crash is that rec_off + 2 is an offset outside the limits of the bnode, and we are writing to it. That's how you get a segfault.
data_off has become 14 because that is the value that is always saved in the last two bytes of the bnode. It is the position of the first record, the weird contant you didn't like in my patch. Now of course, this is wrong, the last two bytes of the bnode should not have been read at all here:
data_off = hfs_bnode_read_u16(node, rec_off);
because rec_off should have been two bytes to the left. But that's a separate consequence of the fact that fd->record should never have been -1 in the first place.
The fact is, size will always be smaller than data_off if the code has no bugs. In fact, when we are deleting the first record of the bnode, data_off - size will be exactly 14 in the first iteration, which is what we want stored in the last two bytes.
Quoted 6 lines
But the
data_offis lesser of size because of value offd->recordwe selected improper position of the offset for the changing the value in the offsets table.What should be done for the issue fix?
(1) We need to add the check condition here:
Quoted 7 lines
http://elixir.free-electrons.com/linux/latest/source/fs/hfsplus/brec.c#L217
do { data_off = hfs_bnode_read_u16(node, rec_off);/*
* TODO: check condition here
* if (data_off< size)
Like I said above, this will never happen if the code is correct, so the logical response here would be BUG(). Think about it, why would the offset of a record that comes after the one we are deleting be smaller than the size of the one we are deleting?
I really don't see any point in having this check at all, though.
Quoted 10 lines
* return error here!!!!!
*/hfs_bnode_write_u16(node, rec_off + 2, data_off - size); rec_off -= 2; } while (rec_off >= end_off);(2) The logic of
hfs_brec_remove()should be reworked here:http://elixir.free-electrons.com/linux/latest/source/fs/hfsplus/brec.c#L197
Quoted 7 lines
/*
* TODO: rework the logic hereif (node->num_recs <= 0) BUG(); /* it could be error message here */ else if (node->num_recs == 1) __hfs_brec_find(node, fd, hfs_find_1st_rec_by_cnid); /* maybe check the error here */
Why are you searching the bnode if you only have one record left? That's the one you want. And fd->key is alredy set to it. Also no, there is no need to check the error, we shouldn't be here at all if there was no record with that cnid.
Quoted 1 line
memcpy(fd->search_key, fd->key, sizeof(hfsplus_btree_key));
This line is the actual fix (close enough to the one in my patch), but it's in the wrong place. If you read my last mail (which I have the feeling you skipped) you'll know that this bug also touches the call to hfs_brec_update_parent() in line 229. So with your solution there will be no segfault, but the parent records won't be updated at all unless they need to be removed.
Also, why are you copying the whole of the hfsplus_btree_key structure, which is like 300 bytes long, when the valid data is usually only in the first 30 bytes or so? I don't think it will make a difference but it seems wasteful, particularly compared to my patch which made use of the existing infrastructure to handle variable length keys.
Quoted 16 lines
--node->num_recs;*/
hfs_bnode_unlink(node); if (!node->parent) return 0; parent = hfs_bnode_find(tree, node->parent); if (IS_ERR(parent)) return PTR_ERR(parent); hfs_bnode_put(node); node = fd->bnode = parent; __hfs_brec_find(node, fd, hfs_find_rec_by_key); goto again; }
Quoted 2 lines
So, we need to extract the key of the latest record from the leaf node by using the
hfs_find_1st_rec_by_cnidstrategy,
Not really. The key of the parent record is also the key of the first record of the child, so you can just copy that, like my patch does.
Quoted 5 lines
copy
fd->keyinto thefd->search_key
and then to go into logic of searching the record in the index node on the basis ofhfs_find_rec_by_keystrategy and the deletion of the record from the index node.What do you think? Could you rework the patch?
Your solution is oddly convoluted, and is equal to my one-line fix in the one thing it does that matters. Yet it still fails to solve half the issue. So I have to say no here.
You can't expect to make a correct patch by just going through callstacks and debug info; that's just dealing with the symptoms. To fix this for real you have to actually figure out what the code is doing.
Quoted 4 lines
Thanks,
Vyacheslav Dubeyko.
Now that you've done your own research: could you please go back to my patch and point out, with your improved understanding of the issue, what it is exactly that you think is wrong with it?
This module does not seem to be getting a lot of attention lately, so I'm very afraid of introducing some nasty bug that will remain in the code for years. I will very much appreciate a careful review.
Ernest